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

Top-level await + dynamic import + cyclic import causes "unsettled TLA" error #55468

Open
guyutongxue opened this issue Oct 20, 2024 · 5 comments

Comments

@guyutongxue
Copy link

guyutongxue commented Oct 20, 2024

Version

v22.10.0

Platform

Linux GuyuDebian 6.1.0-25-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.106-3 (2024-08-26) x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

Create following files:

  • package.json:
{
  "private": true,
  "type": "module"
}
  • foo.js:
const bar = await import("./bar.js");
console.log(bar.default);
  • bar.js:
import {} from "./foo.js";
export default "It works!";

Then run node foo.js.

How often does it reproduce? Is there a required condition?

Every time.

What is the expected behavior? Why is that the expected behavior?

Outputs It works!.

import {} from "./foo.js" doesn't import anything, and foo.js has already been linked, so it should not block the evaluation of bar.js.

What do you see instead?

Warning: Detected unsettled top-level await at file:///home/guyutongxue/Downloads/temp/web/foo.js:1
const bar = await import("./bar.js");

And the process exited with error code 13.

Additional information

Behavior of other runtime:

  • Deno produces same result (error on unsettled TLA).
  • Chromium: stuck on dynamic import.
  • Firefox: also stuck on dynamic import.
  • WinterJS: dynamic import promise rejected with internal error.
  • Safari: outputs It works! successfully.
  • Bun: outputs It works! successfully.

I'm not sure what is the correct behavior that ECMAScript specification defines.

@aduh95
Copy link
Contributor

aduh95 commented Oct 20, 2024

foo.js has already been linked

Sure but linking is only important for static imports, no? Because static imports need to know in advance the exported names of each dependencies.

import {} from "./foo.js" doesn't import anything […], so it should not block the evaluation of bar.js.

The import statement itself is a no-op during evaluation, so it's not blocking anything. However, bar.js source text is never evaluated because it's waiting for its dependencies (i.e. ./foo.js) to be evaluated first, which is blocked on the TLA.
My understanding is that the bug in on Bun, and Node.js behavior matches how I interpret the spec.

@guyutongxue
Copy link
Author

@aduh95 Thanks for your explanation.

However, bar.js source text is never evaluated because it's waiting for its dependencies to be evaluated first ...

But I'm just a bit confused that, in "static" cyclic dependencies, for example,

// a.js
import { b } from "./b.js";
await new Promise((r) => setTimeout(r, 1000));
console.log("module a loaded");

// b.js
import * as a from "./a.js";
console.log(a);
export const b = 1;

The console.log(a) is executed before console.log("module a loaded"). Seems that b.js does not wait the dependency import ... from "./a.js" to finished. So why things got changed in a dynamic import context?

@aduh95
Copy link
Contributor

aduh95 commented Oct 20, 2024

Take the following shell script:

mkdir repro
cd repro
cat -> a.mjs <<'EOF'
import { b } from "./b.mjs";
await new Promise((r) => setTimeout(r, 1000));
console.log("module a loaded");
EOF
cat -> b.mjs <<'EOF'
import * as a from "./a.mjs";
console.log(a);
export const b = 1;
EOF
echo "a.mjs is the entry point:"
node a.mjs
echo
echo "b.mjs is the entry point:"
node b.mjs
cd ..
rm -rf repro

This outputs:

a.mjs is the entry point:
[Module: null prototype] {  }
module a loaded

b.mjs is the entry point:
module a loaded
[Module: null prototype] {  }

As you can see, the order depends on which is the entry point, and the entry point is always executed last.

So why things got changed in a dynamic import context?

Because there's no cyclic module graph when you use dynamic imports. Cyclic module has its own section of the spec, it's not surprising you get different results with static import and dynamic imports.

@Jamesernator
Copy link

Jamesernator commented Oct 21, 2024

So why things got changed in a dynamic import context?

This was considered but ultimately rejected as it was favoured that dynamic import should always return a fully evaluated module.

Spec-wise, it wouldn't actually be that complicated to have an option to import(...) to allow partially initialized namespaces (just like static import already allows), but it would need be a TC39 proposal to actually happen.

Technically I think it would actually be conforming for hosts to have this behaviour by an import attribute (e.g. import("./foo.js", { with: { detectCyclicImportAndReturnPartialNamespace: "true" } })).

I don't know if there's really enough demand that Node (let alone other hosts) would add this (though I have seen basically this exact issue be raised a couple times before, so maybe the demand is high enough, but someone would have to implement it).

@guyutongxue
Copy link
Author

I've encountered this issue because Vite is building TLA + import() to a cyclic module graph.

Original source:

// main.ts
await import("./module_1");

// module_1.ts
import("./moudle_2");

builds to:

// main.js
export const __vite_preload = /* ... */;
await __vite_preload(() => import("./module_1.js"));

// module_1.js
import { __vite_preload } from "./main.js";
__vite_preload(() => import("./module_2.js"));

The corresponding Vite issue is vitejs/vite#18156 -- this issue does affect a number of user.

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

No branches or pull requests

3 participants