avoid top-level dynamic codemirror imports

[thomasheyenbrock]

Fixes #2368

This should fix breaking imports of @graphiql/react (or its dependent graphiql) in non-browser environments like Node.js. Remix for example breaks during SSR because these imports also get loaded and executed. codemirror assumes to always run in a browser env though.

[changeset-bot]

🦋 Changeset detected

Latest commit: f1f8dda

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

This PR includes changesets to release 2 packages

Name	Type
@graphiql/react	Patch
graphiql	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

[github-actions]

The latest changes of this PR are available as canary in npm (based on the declared changesets):

[email protected]
@graphiql/[email protected]

[codecov]

Codecov Report

Merging #2545 (f1f8dda) into main (2d91916) will increase coverage by 3.74%.
The diff coverage is 23.12%.

@@            Coverage Diff             @@
##             main    #2545      +/-   ##
==========================================
+ Coverage   65.70%   69.45%   +3.74%     
==========================================
  Files          85       71      -14     
  Lines        5106     4154     -952     
  Branches     1631     1375     -256     
==========================================
- Hits         3355     2885     -470     
+ Misses       1747     1264     -483     
- Partials        4        5       +1     

Impacted Files	Coverage Δ
packages/codemirror-graphql/src/lint.ts	100.00% <ø> (ø)
packages/codemirror-graphql/src/results/mode.ts	47.05% <ø> (ø)
packages/codemirror-graphql/src/utils/hintList.ts	95.65% <ø> (ø)
...ckages/codemirror-graphql/src/utils/mode-indent.ts	0.00% <0.00%> (ø)
packages/codemirror-graphql/src/variables/mode.ts	79.48% <ø> (ø)
packages/graphiql-react/src/editor/whitespace.ts	100.00% <ø> (ø)
packages/graphiql-react/src/utility/debounce.ts	0.00% <0.00%> (ø)
packages/graphiql-react/src/editor/tabs.ts	5.76% <5.76%> (ø)
packages/codemirror-graphql/src/variables/lint.ts	46.98% <66.66%> (ø)
packages/codemirror-graphql/src/hint.ts	94.73% <100.00%> (ø)
... and 97 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ad0793...f1f8dda. Read the comment docs.

[acao]

Why are they resolving unresolved promises though? The import should only happen on await import() right? Or perhaps native dynamic import in node works differently in the browser?? I am so baffled why they are trying to import these files before the promises are even resolved?

For example, the canonical react.lazy() examples declare unfulfilled dynamic imports this way

[dimaMachina]

Not related to your PR, but why we need it? I guess it should be

Suggested change

	await Promise.all(allAddons.map(addon => addon));
	await Promise.all(allAddons);

[acao]

Yeah this was my original implementation! I wonder why map() was added? import() only resolves one value because its a promise, so I’m not sure what else it would be for

[dimaMachina]

In this place it makes no sens to do it 🙃

[thomasheyenbrock]

Oops, now I forgot to include this 🙈 will follow up in another PR

[thomasheyenbrock]

See #2559

[acao]

Hopefully this resolves the issue!

[thomasheyenbrock]

This has in fact nothing to do with bundlers after thinking about it again.

In the moment you call import() Node.js will dispatch the promise and push it on the call stack. On the next available tick import will load the module, thus executing its content.

As an example, you could do the following in Node.js:

• Have a file dynamic.mjs like this

  console.log("i'm running");

• Have a file index.mjs like this:

  function wait(ms) {
    return new Promise((r) => setTimeout(r, ms));
  }
  
  import("./dynamic.mjs");
  
  wait(2000).then(() => console.log("done"));

What you'll see printed is first i'm running and then done, no matter if you await the dynamic import or not.

[acao]

Indeed! I had a similar realization in the original issue comments. Thus why React.lazy(() => import()) works for top level declarations, because of the function closure!