Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert import order to fix imported class being not yet defined #2599

Merged
merged 2 commits into from
May 18, 2024

Conversation

flovouin
Copy link
Contributor

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
Copy link
Contributor Author

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

@flovouin
Copy link
Contributor Author

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
Copy link
Contributor

@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 dvdsgl enabled auto-merge (squash) May 18, 2024 17:56
@dvdsgl dvdsgl merged commit 1ad532b into glideapps:master May 18, 2024
2 of 23 checks passed
@dvdsgl
Copy link
Member

dvdsgl commented May 18, 2024

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

dvdsgl added a commit that referenced this pull request May 18, 2024
dvdsgl added a commit that referenced this pull request May 18, 2024
@flovouin flovouin deleted the fix/type-utils-import branch May 21, 2024 05:18
@flovouin
Copy link
Contributor Author

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';

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants