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

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

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

jasnell
Copy link
Member

@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
Member 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
Member 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.

src/workerd/api/node/module.c++ Outdated Show resolved Hide resolved
src/workerd/io/compatibility-date.capnp Show resolved Hide resolved
src/workerd/jsg/modules.c++ Show resolved Hide resolved
src/workerd/jsg/modules.c++ Show resolved Hide resolved
src/workerd/jsg/modules.h Show resolved Hide resolved
@jasnell
Copy link
Member 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 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
14 checks passed
@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