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

Use context's globals instead of serializing there #1220

Merged
merged 2 commits into from
May 5, 2024
Merged

Conversation

edemaine
Copy link
Collaborator

@edemaine edemaine commented May 5, 2024

#1219 was kind of ugly: it serialized the serialize function, parsed it in the comptime context, and then ran it there, so that it could have access to the context's globals.

This PR takes what I think is a cleaner approach: run the serialize in our usual context, but give it access to the comptime context's globalThis and check for matching objects from either globalThis.

In both approaches we have to copy globals over to comptime context, because new contexts don't get the usual Node builtins, such as URL. (See nodejs/node#28823) I'm now doing this in the way that node:repl does, which is better: it gives the comptime context access to important Node globals like fetch, atob, process, etc. This also restores Node's global which seems to be global to all modules, whereas globalThis is local to the module, which makes sense and enables some magic interactions.

// one.js
global.x = 5
require('./two.js')
// two.js
console.log(global.x)  // prints 5

@@ -203,22 +194,22 @@ function serialize(value: ???): string
stack.add val
str :=
switch Object.getPrototypeOf val
when RegExp::
when RegExp::, context?.RegExp?::
Copy link
Contributor

Choose a reason for hiding this comment

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

Are both ?s necessary? If context is undefined the rest of the chain is skipped.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair point. I think in the future we might want to add more ?s before ::, for both local and remote checks, but given that we currently require that RegExp (say) is present, we can assume that it's also in context, given how we copy over all globals. (I wonder if there are JS runtimes e.g. with no TypedArray support. But we can wait for that to arise.)

@edemaine edemaine merged commit 9c10ea2 into main May 5, 2024
3 checks passed
@edemaine edemaine deleted the comptime-context branch May 5, 2024 16:10
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