federation icon indicating copy to clipboard operation
federation copied to clipboard

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

Open benjamn opened this issue 2 years ago • 3 comments

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 avatar Feb 01 '24 22:02 benjamn

🦋 Changeset detected

Latest commit: f02243e254aea8d6180b3b4110d98ff7b16cde8d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@apollo/federation-internals Patch
@apollo/gateway Patch
@apollo/composition Patch
@apollo/query-planner Patch
@apollo/query-graphs Patch
@apollo/subgraph Patch
apollo-federation-integration-testsuite Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Feb 01 '24 22:02 changeset-bot[bot]

Deploy Preview for apollo-federation-docs canceled.

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

netlify[bot] avatar Feb 01 '24 22:02 netlify[bot]

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.

codesandbox-ci[bot] avatar Feb 01 '24 22:02 codesandbox-ci[bot]