Skip to content

Disable top-level await in require with a compat flag#3081

Merged
jasnell merged 3 commits intomainfrom
jsnell/disable-top-level-await
Nov 13, 2024
Merged

Disable top-level await in require with a compat flag#3081
jasnell merged 3 commits intomainfrom
jsnell/disable-top-level-await

Conversation

@jasnell
Copy link
Collaborator

@jasnell jasnell commented Nov 8, 2024

Currently, the top level scope of a Worker script and modules that are imported may use top level await. However, because we restrict the use of i/o in the top level scope, the only thing that can be awaited at the top level scope is a dynamic import, which we require to be resolved synchronously. It is a bit nonsensical to support synchronously resolved dynamic imports at the top level when we can just use static imports more effectively. Additionally, the ecosystem and runtimes are moving to a state where top level await in modules is being strongly discouraged. This compat flag. This compatibility flag will make it impossible to use top level await.

(made the change with lots of distractions so keeping as a draft until I can do a second pass to make sure nothing was missed)

Update: Updated to apply the compat flag only to require() calls, along with a few other relevant cleanups. Still keeping as a draft because this will need some extensive testing.

@jasnell
Copy link
Collaborator Author

jasnell commented Nov 9, 2024

After an initial review of impact on this, I think this PR needs to be updated to relax the restriction a bit more. TLA should be restricted only when modules are require(...), but entry point and import would work fine.

@jasnell jasnell force-pushed the jsnell/disable-top-level-await branch 3 times, most recently from 46fa8bc to 51d5c9a Compare November 9, 2024 20:07
@jasnell jasnell changed the title Disable top-level await with a compat flag Disable top-level await in require with a compat flag Nov 10, 2024
@jasnell jasnell force-pushed the jsnell/disable-top-level-await branch 2 times, most recently from 9fcfd0b to 23b4340 Compare November 10, 2024 20:25
@jasnell
Copy link
Collaborator Author

jasnell commented Nov 10, 2024

Updated to further refine and also eliminates the limitation around circular dependencies of commonjs modules. Not marking this "ready for review" just yet because there are a few more additional cleanups I want to do to eliminate duplicated code.

@kentonv

This comment was marked as resolved.

@jasnell
Copy link
Collaborator Author

jasnell commented Nov 11, 2024

@kentonv

There are actually several async APIs that work at startup time, including WebCrypto and WebAssembly. I think there are real use cases for being able to use these at startup so I'm not excited about taking it away.

Those will not be impacted. Note that this PR has been updated to ONLY apply to use of require() and createRequire(). There is zero impact on the worker top-level evaluation and anything imported using static or dynamic import.

@jasnell jasnell force-pushed the jsnell/disable-top-level-await branch 2 times, most recently from 1f182f4 to 57cfd68 Compare November 12, 2024 16:49
@jasnell jasnell marked this pull request as ready for review November 12, 2024 16:50
@jasnell jasnell requested review from a team as code owners November 12, 2024 16:50
@jasnell jasnell requested review from hoodmane and npaun November 12, 2024 16:50
@jasnell jasnell force-pushed the jsnell/disable-top-level-await branch from 57cfd68 to cfca7ac Compare November 12, 2024 20:11
@jasnell jasnell force-pushed the jsnell/disable-top-level-await branch from cfca7ac to e858332 Compare November 12, 2024 21:02
@jasnell jasnell merged commit 12479a1 into main Nov 13, 2024
@jasnell jasnell deleted the jsnell/disable-top-level-await branch November 13, 2024 15:05
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.

3 participants