ref(nextjs): Use proxy loader for wrapping all data-fetching functions #5602
Conversation
There was a problem hiding this comment.
Best. Change. Ever. I'm super impressed that you managed to make it very clear what's happening, given that all this is rather complicated. Glad to see this "theory" become reality.
See this PR more or less as approved. Comments are either questions or nits.
There is one high-level topic I wanna discuss before actually pressing the green checkmark button (see the comment about maybe bundling rollup in the loader).
| exports: 'named', | ||
| }, | ||
| external: ['@rollup/plugin-sucrase', 'rollup'], |
There was a problem hiding this comment.
Just had a thought. Considering we depend on a very specific behaviour of rollup (i.e. the unwrapping of the * in export * from ...), what do you think about doing the following two things:
- We pin the exact version of rollup we're using so this behaviour doesn't just randomly break with an update.
- We bundle
rollupwithin our loader so that pinning the version doesn't turn into a dependency fiasco for our downstream users.
There was a problem hiding this comment.
-
Pinning I think is a good idea (which is why I did it! 😛)
-
You mean like vendoring the code ourselves? What kind of fiasco are you picturing?
There was a problem hiding this comment.
- Sorry I am dumb. Sometimes im just writing down thoughts as suggestions without checking if they arent already there. Good that we're on the same page there.
- I am double dumb. The dep problem i was thinking of is with peer deps and not normal deps. I think we're good here!
There was a problem hiding this comment.
LOL. Terrific. Now we're even for the other day when I entirely forgot the wrappers existed for about five minutes. And I'm the one who originally wrote them!
| // Nextjs uses square brackets surrounding a path segment to denote a parameter in the route, but rollup turns those | ||
| // square brackets into underscores. Further, Rollup adds file extensions to bare-path-type import and export sources. |
There was a problem hiding this comment.
rollup turns those square brackets into underscores
wat
There was a problem hiding this comment.
Yeah, I know. I don't understand how that could possibly work, for anyone. I feel like it's gotta be a bug, which I should probably report...
lobsterkatie
force-pushed
the
kmclb-nextjs-use-proxy-loader-for-all-wrapping
branch
from
1691668
to
6b358f7
Compare
lobsterkatie
force-pushed
the
kmclb-nextjs-use-proxy-loader-for-all-wrapping
branch
from
6b358f7
to
fe67cdb
Compare
size-limit report 📦
|
Yeah, I'm actually pretty pleased with how clean it was in the end. It was a very good idea you had! |
lobsterkatie
deleted the
kmclb-nextjs-use-proxy-loader-for-all-wrapping
branch
This changes the experimental auto-wrapping feature to use a templated proxy module for all wrapping. (Previously, the wrapping was done using a different mix of code parsing, AST manipulation, proxying, templating, and string concatenation for each function.) Not only should this be easier to reason about and maintain (because it's one unified method of wrapping), it also solves a number of the disadvantages inherent in various parts of the previous approach. Specifically, using a template for everything rather than storing code as strings lets us take advantage of linting and syntax highlighting, and using a proxy loader rather than dealing with the AST directly puts the onus of handling syntax variations and edge cases on tools which are actually designed for that purpose.
At a high level, the proxy loader works like this:
pagesdirectory, and feeds each one to our loader.Previously, when working directly with the page module's AST, we had to account for the very many ways functions can be defined and exported. By contrast, doing the function wrapping in a separate module allows us to take advantage of the fact that imported modules have a single, known structure, which we can modify directly in the template code.
Notes:
For some reason, nextjs won't accept data fetchers which are exported as part of an
export * from '...'statement. Therefore, the "re-export everything" part of the third step above needs to be of the formexport { all, of, the, things, which, the, page, module, exports, listed, individually } from './pageModule'. This in turn requires knowing the full list of each page module's exports, since, unfortunately,export { ...importedPageModule }isn't a thing. As it turns out, one of the noticeable differences between our published code before and after the build process revamp in the spring is that wheretscleavesexport *statements untouched, Rollup splits them out into individual exports - exactly what's needed here! The loader therefore uses Rollup's JS API to process the proxy module code before returning it. Further, in order that Rollup be able to understand the page module code (which will be written in eitherjsxortsx), we first use Sucrase to transpile the code to vanilla JS. Who knew the build process work would come in so handy?Given that we replace a page module's contents with the proxy code the first time webpack tries to load it, we need webpack to load the same module a second time, in order to be able to process and bundle the page module itself. We therefore attach a query string to the end of the page module's path wherever it's referenced in the template, because this makes Webpack think it is a different, as-yet-unloaded module, causing it to perform the second load. The query string also acts like a flag for us, so that the second time through we know we've already handled the file and can let it pass through the loader untouched.
Rollup expects the entry point to be given as a path to a file on disk, not as raw code. We therefore create a temporary file for each page's proxy module, which we then delete as soon as rollup has read it. The easiest way to make sure that relative paths are preserved when things are re-exported is to put each temporary file alongside the page module it's wrapping, in the
pages/directory. Fortunately, by the time our loader is running, Webpack has already decided what files it needs to load, so these temporary files don't get caught up in the bundling process.In order to satisfy the linter in the template file, the SDK itself has been added as a dev dependency. Fortunately this seems not to confuse yarn.
Just to keep things manageable, this stops using but doesn't yet delete the previous loader (and associated files/code). Once this is merged, I'll do that in a separate PR.
Ref #5505