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.

Sign up for GitHub

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

Jump to bottom

fix(core): allow esm extensions not included in snapshot #18980

Merged
merged 5 commits into from
May 4, 2023
Merged

fix(core): allow esm extensions not included in snapshot #18980

merged 5 commits into from
May 4, 2023

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented May 3, 2023 •
edited
Loading

Fixes #18979.

This changes the predicate for allowing ext: specifier resolution from snapshot_loaded_and_not_snapshotting to ext_resolution_allowed which is only set to true during the extension module loading phase. Module loaders as used in core
are now declared as ExtModuleLoader rather than dyn ModuleLoader.

nayeemrmn
nayeemrmn commented May 3, 2023
View reviewed changes
core/runtime.rs Outdated Show resolved Hide resolved
bartlomieju
bartlomieju reviewed May 3, 2023
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great cleanup, you simplified this subsystem a lot. I got a few minor concerns

bench_util/js_runtime.rs Outdated Show resolved Hide resolved
cli/tests/testdata/run/extension_dynamic_import.ts.out Show resolved Hide resolved
core/modules.rs Outdated Show resolved Hide resolved
core/runtime.rs Outdated Show resolved Hide resolved
core/runtime.rs Show resolved Hide resolved
bartlomieju
bartlomieju approved these changes May 4, 2023
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @nayeemrmn. Great work!

@bartlomieju bartlomieju merged commit 7a8bb3b into denoland:main May 4, 2023
@nayeemrmn nayeemrmn deleted the extension-esm-resolution branch May 4, 2023 00:50
levex pushed a commit that referenced this pull request May 4, 2023
@nayeemrmn @levex
fix(core): allow esm extensions not included in snapshot (#18980)
979742d 
Fixes #18979.

This changes the predicate for allowing `ext:` specifier resolution from
`snapshot_loaded_and_not_snapshotting` to `ext_resolution_allowed` which
is only set to true during the extension module loading phase. Module
loaders as used in core
are now declared as `ExtModuleLoader` rather than `dyn ModuleLoader`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

deno_runtime's WorkerOptions::extensions breaks with esm extensions
2 participants
@nayeemrmn @bartlomieju