Allow omitting name while creating dynamic workers #5574
Conversation
|
The generated output of |
ketanhwr
force-pushed
the
ketan/dyworker-noname
branch
from
2568333 to
df2bb8e
Compare
| // Use a dynamic Worker loader binding to obtain an Worker by name. If the named Worker | ||
| // doesn't already exist, the callback will be called to fetch the source code from which the | ||
| // Worker should be created. | ||
| // Use a dynamic Worker loader binding to obtain an Worker by name. If name is undefined, or if |
There was a problem hiding this comment.
| // Use a dynamic Worker loader binding to obtain an Worker by name. If name is undefined, or if | |
| // Use a dynamic Worker loader binding to obtain an Worker by name. If name is null, or if |
| kj::Own<WorkerStubChannel> loadIsolate( | ||
| kj::String name, kj::Function<kj::Promise<DynamicWorkerSource>()> fetchSource) { | ||
| kj::Maybe<kj::String> name, kj::Function<kj::Promise<DynamicWorkerSource>()> fetchSource) { | ||
| auto actualName = kj::mv(name).orDefault([] { return randomUUID(kj::none); }); |
There was a problem hiding this comment.
I think if we just don't put the WorkerStubImpl into the map here, but instead just construct it and return it, everything should work fine. Then we don't need to assign a random name?
There was a problem hiding this comment.
Both WorkerStubImpl's constructor, and the subsequent call to server.makeWorkerImpl unfortunately need a name. Agreed on not inserting into the map here though
There was a problem hiding this comment.
I took a quick look and the only place I can see that actually requires a unique name is this code:
KJ_IF_SOME(isolateRegistrar, inspectorIsolateRegistrar) {
isolateRegistrar->registerIsolate(name, isolate.get());
}
But I suppose it is probably useful for logging and stuff for it to be unique.
Maybe the name should be dynamic:<uuid>, which makes the logging a little clearer.
| assert.strictEqual(loadCount, 5); | ||
| // Test that null/undefined name parameter creates unique isolates each time | ||
| // Each call should generate a unique name internally. | ||
| let noName1 = env.loader.get(null, loadCodeCallback).getEntrypoint(); |
There was a problem hiding this comment.
Feel free to ignore this, but what about allowing env.loader.get(loadCodeCallback).getEntrypoint()?
There was a problem hiding this comment.
The original version of this PR reversed the ordering of the params to make the param optional, but that was a breaking change.
But we could, perhaps, create an "overloaded" interface, which accepts either (id, func) or (func). But overloads are kind of awkward.
And actually, if you think about it, when the ID is null, then there's no need for a callback. The callback will always be called, so instead we should just let you pass in the worker definition instead of a callback.
So maybe what we really want is a new method like: load(code)
Which is logically equivalent to: get(null, () => code)
Either way I think passing null to get() should be supported. But probably we want to add this new load() method as well.
3 participants
No description provided.