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

Fix module load order within internals-js/src/specs directory using index.ts module #2924

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Feb 1, 2024

While attempting to write new tests in the internals-js/src/specs/__tests__/ directory, I found some circularities in the import structure of the FeatureDefinition subclasses, triggered by importing a different entry point module than usual.

See for example this CI run which fails with the following error:

 FAIL  internals-js/src/specs/__tests__/sourceSpec.test.ts
   Test suite failed to run

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

      35 | export const inaccessibleIdentity = 'https://specs.apollo.dev/inaccessible';
      36 |
    > 37 | export class InaccessibleSpecDefinition extends FeatureDefinition {
         |                                                 ^
      38 |   public readonly inaccessibleLocations: DirectiveLocation[];
      39 |   public readonly inaccessibleDirectiveSpec: DirectiveSpecification;
      40 |   private readonly printedInaccessibleDefinition: string;

      at Object.<anonymous> (src/specs/inaccessibleSpec.ts:37:49)
      at Object.<anonymous> (src/definitions.ts:45:1)
      at Object.<anonymous> (src/specs/coreSpec.ts:3:1)
      at Object.<anonymous> (src/specs/sourceSpec.ts:2:1)
      at Object.<anonymous> (src/specs/__tests__/sourceSpec.test.ts:1:1)

If you examine that stack trace, the crux of the problem is the inaccessibleSpec.ts module attempts to import FeatureDefinition from coreSpec.ts while the coreSpec.ts module is still being evaluated, so FeatureDefinition has not been exported yet from coreSpec.ts at the moment InaccessibleSpecDefinition tries to extend it.

I'll be the first to argue import cycles are not necessarily evil in JavaScript (ES2015+), but cycles do potentially lead to different module load order depending on which module you import first, and the internals-js/specs/*Spec.ts code is especially sensitive to load order because of synchronous use of imports like FeatureDefinition in module scope.

There are some tricks for getting extends FeatureDefinition not to complain if FeatureDefinition is undefined (until the first instance of the subclass is created), but, based on some light internet research, a more robust solution is to fix the load order among the sensitive modules using a specs/index.ts module that re-exports everything from the other *Spec.ts modules, and then make sure other code only imports from that specs/index.ts module, rather than importing from individual internals-js/specs/*Spec.ts modules. Here's a good writeup of this approach.

I'll be pushing commits incrementally so you can see the errors before/after I fix them.

@benjamn benjamn self-assigned this Feb 1, 2024

This comment was marked as resolved.

Copy link

netlify bot commented Feb 1, 2024

Deploy Preview for apollo-federation-docs canceled.

Name Link
🔨 Latest commit f02243e
🔍 Latest deploy log https://app.netlify.com/sites/apollo-federation-docs/deploys/65bc1d8c8803fe0008817eaf

Copy link

codesandbox-ci bot commented Feb 1, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

…n `federationSpec.ts`.

By storing `schema._federationMetadata` as a typed private property of
`Schema`, we can access it using the `schema['_federationMetadata']`
syntax in places where we don't want to import the `federationMetadata`
helper function, since doing so also imports the whole `federation.ts`
module, leading to dependency cycles.
@benjamn benjamn changed the title Test file that triggers module import order problems. Fix module load order within internals-js/src/specs directory using index.ts module Feb 1, 2024
@benjamn benjamn marked this pull request as ready for review February 1, 2024 22:40
@benjamn benjamn requested a review from a team as a code owner February 1, 2024 22:40
@clenfest clenfest removed the request for review from trevor-scheer March 12, 2024 16:37
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.

None yet

1 participant