Revert import order to fix imported class being not yet defined

flovouin commented 1 month ago

Description

This PR fixes a circular dependency issue following ESlint-related changes in #2558.

Using quicktype-core in my own project crashes at runtime because of this issue since version 23.0.159 with the following error:

TypeError: Class extends value undefined is not a constructor or null

      at Object.<anonymous> (../node_modules/quicktype-core/dist/GraphRewriting.js:143:53)
      at Object.<anonymous> (../node_modules/quicktype-core/dist/TypeGraph.js:9:26)
      at Object.<anonymous> (../node_modules/quicktype-core/dist/Type.js:10:21)
      at Object.<anonymous> (../node_modules/quicktype-core/dist/TypeBuilder.js:10:16)
      at Object.<anonymous> (../node_modules/quicktype-core/dist/attributes/StringTypes.js:7:23)
      at Object.<anonymous> (../node_modules/quicktype-core/dist/TypeUtils.js:7:23)

There are many ESlint import/no-cycle errors being disabled in the codebase. I wouldn't be surprised if other refactored imports were problematic. However fixing this one was enough in my case.

I apologise for the lack of reproduction example and addition of tests. However it seems that only "end to end" tests for each language exist. I did run the existing tests and linter, though.

Related Issue

This follows the changes in #2558.

Motivation and Context

I can no longer use quicktype-core as a dependency since version 23.0.159.

Previous Behaviour / Output

TypeError: Class extends value undefined is not a constructor or null

      at Object.<anonymous> (../node_modules/quicktype-core/dist/GraphRewriting.js:143:53)
      at Object.<anonymous> (../node_modules/quicktype-core/dist/TypeGraph.js:9:26)
      at Object.<anonymous> (../node_modules/quicktype-core/dist/Type.js:10:21)
      at Object.<anonymous> (../node_modules/quicktype-core/dist/TypeBuilder.js:10:16)
      at Object.<anonymous> (../node_modules/quicktype-core/dist/attributes/StringTypes.js:7:23)
      at Object.<anonymous> (../node_modules/quicktype-core/dist/TypeUtils.js:7:23)

New Behaviour / Output

My project relying on quicktype-core passes its test suite.

How Has This Been Tested?

I patched the quicktype-core distribution in my own project with the change in this PR.

Screenshots (if appropriate):

N/A

flovouin commented 1 month ago

cc @inferrinizzard and @dvdsgl, who wrote and reviewed #2558.

flovouin commented 1 month ago

By digging a bit deeper, I found a way to avoid the circular dependency issue as a user of this package: always ensure the "root" quicktype-core module is imported first. In some source files, I was only importing some utilities that are not exported in the package's entrypoint, e.g.:

import { removeNullFromType } from 'quicktype-core/dist/TypeUtils.js';

Changing the import to:

import 'quicktype-core'
import { removeNullFromType } from 'quicktype-core/dist/TypeUtils.js';

ensures dependencies are loaded in order.

I guess there are two ways to solve this problem: fixing circular dependencies in quicktype-core (this PR fixes one), or exposing more utilities in the index.ts file of the package. However the latter requires a lot more work, because there are many utilities that are useful (if not required) when implementing code generation for a new language using quicktype-core.

inferrinizzard commented 1 month ago

@flovouin thanks for the PR! Dependency cycles were definitely something I wanted to address after migrating to ESLint so thank you for starting that effort

exposing more utilities in the index.ts file of the package. However the latter requires a lot more work, because there are many utilities that are useful (if not required) when implementing code generation for a new language using quicktype-core.

^ This would probably end up being a better long term path once I finalise a guide on how to more easily create new custom language and behaviours - what specific utilities are you using that aren't exported yet ?

dvdsgl commented 1 month ago

@inferrinizzard let's take a look at our automerge settings – this merged itself after failing.

flovouin commented 1 month ago

Thanks!

what specific utilities are you using that aren't exported yet ?

Here's what I found in my codebase:

import { RenderContext, Renderer } from 'quicktype-core/dist/Renderer.js';
import { SourcelikeArray } from 'quicktype-core/dist/Source.js';
import { descriptionTypeAttributeKind, propertyDescriptionsTypeAttributeKind } from 'quicktype-core/dist/attributes/Description.js';
import { AcronymStyleOptions } from 'quicktype-core/dist/support/Acronyms.js';
import { ConvertersOptions } from 'quicktype-core/dist/support/Converters.js';
import { removeNullFromType } from 'quicktype-core/dist/TypeUtils.js';

glideapps / quicktype