[POC] Generic code patching #741
Conversation
|
commit: |
|
Glanced at the code, looks great. |
| @@ -36,7 +40,7 @@ export async function createServerBundle(options: buildHelper.BuildOptions) { | |||
| if (fnOptions.runtime === "edge") { | |||
| await generateEdgeBundle(name, options, fnOptions); | |||
There was a problem hiding this comment.
Should we pass patchers for the edge bundle too?
There was a problem hiding this comment.
I don't think so, or at least not in its current form. For the edge runtime there is not really a concept of traced files, and given that we are already using esbuild for all the files it probably make more sense to use esbuild plugin there.
In fact we could also allow to pass additional esbuild plugin in server bundle (i think that's all that's really needed for you to use createServerBundle directly right ?
There was a problem hiding this comment.
Makes sense.
i think that's all that's really needed for you to use createServerBundle directly right ?
One other thing is that we include the edge ON config.
I am thinking that maybe we should have a deployTo: cloudflare|node in the config so that we could handle in this in a generic way. I still need to think a bit more about that.
FYI what I'd like to look at next:
- change
internalEvent.urlto always be resolved URL (vs a relative URL) - this is blocking us from using bundled middleware because external middleware needs a resolved URL vs server bundle require a path. Using a bundled middleware would reduce the bundle size when the middleware and server are in the same worker - What I'm thinking is that we could compile an
init.jsand use that instead of the ESBuild banners. That init would depend on the platform (i.e.deployTo) - With cloudflare workers, the main reason why I'm looking at this is to allow accessing
process.envandgetCloudflareContext()at top level.
Sorry if this is no very detailed and a little confusing, I'm still thinking about how all of this should be done. I know where I want to go but not exactly sure about the path. It should hopefully become clearer in the next few days.
There was a problem hiding this comment.
IMO this shouldn't be in the config but something in the wrapper itself, right now it returns things like supportStreaming, we could add a deployTo or platformRuntime field there. The wrapper is already dependent on the runtime it is deployed to anyway
| contentFilter?: RegExp | VersionedField<RegExp>[]; | ||
| patchCode: PatchCodeFn | VersionedField<PatchCodeFn>[]; |
There was a problem hiding this comment.
I'm afraid having multiple VersionnedField (with potentially different versions) might be a mess, should before/after be member of CodePatcher to make things simpler?
There was a problem hiding this comment.
The idea behind it is to more easily accomodate small changes while keeping the reason why we need things like that.
For example if next just move a file, we just need to make a small change to filter and it makes it very clear why.
But i admit that it might be a little bit harder to understand at first glance than having just multiple CodePatcher
| buildHelper.compareSemver(version, field.before) >= 0 && | ||
| buildHelper.compareSemver(version, field.after) < 0 | ||
| ) { | ||
| result.push(field.field); |
There was a problem hiding this comment.
Nits:
Does the result need to be an array?
You can also return early to avoid else
There was a problem hiding this comment.
If we keep VersionedField as they are i think it is, we may need to have multiple field that match. For example in one version of next there is 2 bits of code to fix, 1 in another and maybe 3 in the last , with this we can have 3 PatchCodeFn with just the needed version
There was a problem hiding this comment.
That's quite nice and generic!
I understand that VersionnedField[] allow for a single patch to cover different versions but I wonder if it's worth the burden (maintenance, readability).
Maybe only adding before/after to a patch could be enough?
Then i.e. pathAsyncLocalStroage could be an array of code patches for different versions
|
One thing that would be nice is for patched to be able to report a status (messages, stats). |
| @@ -66,6 +66,7 @@ export async function createMiddleware( | |||
| additionalExternals: config.edgeExternals, | |||
| onlyBuildOnce: forceOnlyBuildOnce === true, | |||
| name: "middleware", | |||
| additionalPlugins: [], | |||
There was a problem hiding this comment.
one caveat with ESBuild plugins is that onLoads are called only until one returned a value (!== undefined).
We implemented a ContentUpdated.It would allow updating a file i.e. according to the Next version and the platform (workers vs node) with 2 different plugins.
There was a problem hiding this comment.
I'm not sure i totally understand what you suggest here. You'd want to move ContentUpdater here with a few additional check like VersionedField ? And instead of additionalPlugins being Plugin[], it would be ((updater: ContentUpdater) => Plugin)[] ?
Anyway after merging this and using it in cloudflare, you'll likely don't need the ContentUpdater anymore, pretty much all the plugins that use it today (and some that don't) could be migrated to CodePatcher
This is just a proposal of how we could introduce code patching for next code directly here.
First it would only be available internally but we could introduce later something like an
open-next.build.tsfile that would allow users or adapter based on us to use this more easily.@vicb @dario-piotrowicz @james-elicx i'd appreciate some early feedback on this. It should be possible to migrate some of the patch in cloudflare to use this.