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

fix: preserve cped in dynamic imports #888

Merged
merged 1 commit into from
Sep 10, 2024
Merged

fix: preserve cped in dynamic imports #888

merged 1 commit into from
Sep 10, 2024

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Aug 29, 2024

Fix for denoland/deno#25275

This code is like hacks on top of hacks, but basically save & restore cped in the dynamic import flow.

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.

Yikes, this is indeed gnarly. Any chance you could add a test for that?

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

So FYI, real AsyncContext will likely not do referrer based inheritance of the context, but instead will run the module evaluation (for all modules) in the root context.

This is because otherwise the context used in the module is racy - it's unfortunately Node does something different with ALS here.

@devsnek
Copy link
Member Author

devsnek commented Aug 30, 2024

I don't think there's a good way to test this in core without rewriting the module resolver that the testing infra uses to be much more complex... but I will add a test to deno.

@devsnek devsnek merged commit 688c93f into main Sep 10, 2024
18 checks passed
devsnek added a commit to denoland/deno that referenced this pull request Sep 12, 2024
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