Merged
Conversation
2ae43f0 to
47b563c
Compare
Collaborator
Yeah, the new module registry set up works quite a bit differently. Really happy to see these cleanups as I think it'll simplify a number of things. Should be able to do some review here this afternoon |
jasnell
reviewed
May 9, 2025
jasnell
approved these changes
May 9, 2025
Collaborator
jasnell
left a comment
There was a problem hiding this comment.
Overall change LGTM. I'll look at the new module registry details to see if any of that needs to be updated or can be updated to benefit here.
Previously, they were passed to `WorkerdApi::extractSource()` (and from there to `compileModules()`). This is a step towards making `compileModules()` a virtual method on `Worker::Api`.
Honestly this `bool isPython;` member variable declaration hidden between two type declarations in the public area looked like a mistake, but when I deleted it I discovered it was actually used! This is obviously a gross C++ style violation, so let's clean in up.
Instead, it contains a representation of the source code. This refactoring is needed to support dynamic code loading, which won't necessarily originate from a workerd config. This diff is large, but it's mostly just changing code which previosuly switched on config::Worker::Module::Reader (a capnp union) to instead switch on Worker::Script::Module (a OneOf). The details are straightforward.
It isn't needed in workerd, but it will be needed in the internal codebase to get the LimitEnforcer.
This isn't actually needed (at present) in workerd, but the internal codebase needs to implement the same interface, so will need to pass this in somehow.
These aren't currently supported in workerd, but again, the internal runtime needs these to be included in the input to compileModules().
93f800b to
f2858bc
Compare
kentonv
added a commit
that referenced
this pull request
May 11, 2025
…Source. This is a continuation of #4082. I had left `ScriptSource` still containing a weird callback because I didn't think I needed to update it. Turns out I do actually need to update it to avoid some ugliness later. Luckily this turned out to be a much smaller refactor. I would have done it in the first place had I realized.
kentonv
added a commit
that referenced
this pull request
May 19, 2025
Similar to #4082, we want the representation of the worker definition passed to `makeWorker()` to not have to come from a workerd config file. This introduces an intermediate representation, `WorkerDef`, to represent this. No logic has changed here, code has just moved around.
kentonv
added a commit
that referenced
this pull request
May 23, 2025
…Source. This is a continuation of #4082. I had left `ScriptSource` still containing a weird callback because I didn't think I needed to update it. Turns out I do actually need to update it to avoid some ugliness later. Luckily this turned out to be a much smaller refactor. I would have done it in the first place had I realized.
kentonv
added a commit
that referenced
this pull request
Jun 15, 2025
Similar to #4082, we want the representation of the worker definition passed to `makeWorker()` to not have to come from a workerd config file. This introduces an intermediate representation, `WorkerDef`, to represent this. No logic has changed here, code has just moved around.
kentonv
added a commit
that referenced
this pull request
Jun 17, 2025
Similar to #4082, we want the representation of the worker definition passed to `makeWorker()` to not have to come from a workerd config file. This introduces an intermediate representation, `WorkerDef`, to represent this. No logic has changed here, code has just moved around.
kentonv
added a commit
that referenced
this pull request
Jun 17, 2025
Similar to #4082, we want the representation of the worker definition passed to `makeWorker()` to not have to come from a workerd config file. This introduces an intermediate representation, `WorkerDef`, to represent this. No logic has changed here, code has just moved around.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
My goal is to be able to construct a
Worker::Scriptfrom code that does not originate from a workerd config file.To that end, I've defined an intermediate representation of workerd modules as a
OneOfof a bunch of structs (seeWorker::Script::Module).Worker::Script::ModulesSorucenow returns this representation, instead of returning a callback used to compile the modules later.Wroker::Apinow has a virtualcompileModules()method which replaces the old lambda callback, taking the intermediate representation as input.Note: I am a little confused about how the "new module registry" stuff works, since it appears that the code is passed directly to it, bypassing the
Worker::Scriptconstructor. Is the code passed to the constructor not used anymore when using the new module registry? Well, with this change, the code inWorkerdApi::initializeBundleModuleRegistry()is no longer workerd-specific, so perhaps in a future change it could move somewhere else and become reusable in the internal codebase.