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 does not work with dynamic await import() #7649

Closed
TooTallNate opened this issue Sep 23, 2020 · 8 comments · Fixed by #7672 or #7946
Closed

Top-level await does not work with dynamic await import() #7649

TooTallNate opened this issue Sep 23, 2020 · 8 comments · Fixed by #7672 or #7946
Labels
bug Something isn't working correctly cli related to cli/ dir deno_core Changes in "deno_core" crate are needed

Comments

@TooTallNate
Copy link

TooTallNate commented Sep 23, 2020

// mod.ts
const sleep = (n: number) => new Promise(r => setTimeout(r, n));

await sleep(100);

export default 1;
// run.ts

// Works
// import mod from './mod.ts';
// console.log(mod);

// Doesn't work:
//
//   error: Uncaught ReferenceError: default is not defined
const mod = await import('./mod.ts');
console.log(mod);

// Also does not work
//
// import('./mod.ts').then(mod => {
//   console.log(mod);
// });

Running:

$ deno run --allow-read run.ts
error: Uncaught ReferenceError: default is not defined
    at Function.keys (<anonymous>)
    at inspectRawObject (rt/02_console.js:675:31)
    at inspectObject (rt/02_console.js:778:14)
    at inspectValue (rt/02_console.js:429:16)
    at inspectArgs (rt/02_console.js:877:16)
    at Object.log (rt/02_console.js:918:9)
    at file:///Users/nrajlich/deno-bug/run.ts:8:9

Seems like the import() function is not properly waiting for the module to become resolved, so there is no default property on the object returned from import().

Initially reported in: vercel-community/deno#40

@kitsonk kitsonk added bug Something isn't working correctly cli related to cli/ dir deno_core Changes in "deno_core" crate are needed labels Sep 24, 2020
@lucacasonato
Copy link
Member

lucacasonato commented Sep 24, 2020

Hey @MylesBorins - can you take a look at this issue? How are TLA and dynamic imports meant to interact? Should this example work? Thanks :-)

@MylesBorins
Copy link

I can confirm that one should be able to await a dynamic import, that was one of the motivating use cases in the original proposal (although not to be abused, please still use static import too!)

I can confirm that the same code, with slight modifications, works in node.js v14.12.0

@kitsonk
Copy link
Contributor

kitsonk commented Sep 24, 2020

Yeah, I don't think it is TLA. As @TooTallNate mentions, the module isn't ready when the promise is resolved even using the .then(). Clearly we have something messed up in the module instantiation bit with v8, where we are prematurely resolving the promise. I haven't looked in detail but if IIRC, module instantiation is two pass, and I suspect we are resolving the promise for the importing module, and allowing it to be instantiated when only the dependency analysis phase is completed and not waiting for the instantiation.

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Sep 24, 2020

Yeah, I don't think it is TLA. As @TooTallNate mentions, the module isn't ready when the promise is resolved even using the .then().

The issue refers to the TLA in the imported module, I'm pretty sure. The dynamic import promise resolves prematurely when the imported module's execution hits the TLA for the timeout. This might be reproducible with static imports...

Also, it seems it's only reproduced by awaiting a new macrotask or more 😕

@kitsonk
Copy link
Contributor

kitsonk commented Sep 24, 2020

Ah yeah, I see... The real bug is, dynamically importing a module which has TLA fails (which is exactly what the title says 😱 ). So it feels like we are resolving promises out of order then.

@nuxodin
Copy link

nuxodin commented Sep 25, 2020

I think this is the same:
#5215 "nested top level await dynamic imports broken"

@bartlomieju
Copy link
Member

Me and @piscisaureus investigated the issue and arrive at the conclusion that deno_core's module loading need a bigger overhaul to fix this problem. I'll be working towards it.

@TooTallNate
Copy link
Author

Should this be re-opened considering that #7672 was reverted?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly cli related to cli/ dir deno_core Changes in "deno_core" crate are needed
Projects
None yet
7 participants