ref(nextjs): Use virtual file for proxying in proxy loader #5960
Conversation
There was a problem hiding this comment.
This looks good so far. Two questions:
-
I'm not sure I understand the logic of adding the query string in the templates. Is it necessary for the virtual plugin to work? It seems like it will result in a double query string, given that we are also still adding it in in the loader. (I'm wondering if this is related to why you removed the comment explaining why we're adding it in - has the overall system changed somehow so it no longer applies? If so, can you please replace it with a comment which explains how it works now?)
-
I'm trying to figure out what might trigger the screwed-up imports I was running into when I tried this the first time, so we can make sure to test that here. Have you tested this out with pages which are nested a few levels deep? (It's the only thing that's coming to mind at the moment as something which might have been an issue, unless maybe figuring out the business with
makeAbsoluteExternalsRelativealso would have fixed the import problems with the virtual plugin, and it's just that by that point I'd already moved on? 🤔)
Oh, I changed it so we're not adding it in the loader. We're only adding it for the
Yeah I couldn't figure out either how or why that happens. I did in fact test it with nested pages and it still always resulted in " |
|
Sorry, I'm still confused.
I did notice (which I didn't when I first looked at this) that there's both
But... there's nothing removed from the loader code having to do with adding a query string flag. So I'm not sure what you mean by changing it to not add it in the loader or doing it in the template instead of doing it in code. Am I missing something?
Yeah, I guess if it works, it works. I'm just puzzled because other than the double flag business discussed above, I would say that what you have here is more or less identical to what I tried which didn't work. That's why I'm wondering if maybe I gave up in the interest of time before solving some other problem (like the |
I updated the PR description with a "How?" section to describe the changes in more detail. I don't know how else to solve this. Unless there are any major concerns other than it's confusing I would just ship this. |
…-without-writing-to-file
There was a problem hiding this comment.
I updated the PR description with a "How?" section to describe the changes in more detail.
Thank you!
We don't need rollup to process the import statement (1) because wildcard imports are allowed in Next.js. For this reason, this PR adds a query string to the import statement (import * as wrapee from 'RESOURCE_PATH?sentry_external';) telling rollup to ignore this import and to treat it as some external module.
Ohhhhhhh. Okay. Yes, now I get it - we are treating the import differently from the export, with one getting query-stringed before rollup runs and one getting query-stringed after it does so, so that it will process them entirely separately, leaving the import alone while exploding the export * out into its component parts.
This also has the upside that rollup won't mess with the import path by turning it into a relative path that doesn't exist or anything similar.
And yes, right - I'll bet that's why my paths were getting messed up when I tried doing this before. Nice work figuring it out!
So then my only other question is, do the querystring flags need to be different? (This is just for my understanding. Even if they could be the same flag, I think their current names are better because the two flags really do mark two distinct things.)
They don't need to be different. I simply made them different because it might be easier to understand when reading the template files. Small disclaimer here though: I don't know if different query strings might affect caching when resolving/loading files. If somebody complains that their builds suddenly got way slower we might need to revert to identical query strings. |
…5960)" (#6018) In #5960, we switched from writing temporary files to disk to using virtual files when creating proxy modules in the nextjs SDK's proxy loader. Though it was a good change, it didn't handle certain cases with the potential to crash a user's build process. Specifically, rollup converts the absolute paths in the template's `import * as wrappedFile from <wrapped file>` and `export * from <wrapped file>` into relative paths, but can break them in the process, by doing things like replacing `[` and `]` with `_`. Prior to the switch to virtual files, those relative paths only ever had one segment, and so the code compensating for rollup's weirdness only handles basenames. After the switch, the relative paths sometimes have multiple segments, in a pattern which isn't easy to predict. When that happens, the aforementioned compensation code fails and the broken relative paths aren't fixed. After discussing with @lforst, the author of that PR, we agreed to take a different approach to handling the broken paths. Rather than add a fix on top of that change, I'm choosing to revert it and implement the new approach separately, so that the new work is easier to review and the change is easier to reason about. Otherwise, the fix would include a bunch of undoing of changes, which would muddy the waters. #5960 also contained a few small refactors which don't bear directly on the switch to using virtual files, and reverting allows me to pull those out into a separate PR, again for ease of understanding. Reverts 2d068e9.
This makes a few small changes to the `rollupize` function used by the nextjs SDK's proxy loader: - Consolidate error handling in the proxy loader, rather than handling errors inside of `rollupize` and then handling an undefined `rollupize` return value in the proxy loader. - Slightly simplify explanation of `makeAbsoluteExternalsRelative` - Remove type hack in `getRollupInputOptions` which seems no longer to be needed. (It was never clear why it was necessary in the first place, and TS no longer seems mad when it's removed.) - Rename `resourcePath` to `userModulePath` for clarity The latter two changes are drawn from #5960, to preserve them even though it is being reverted.
…6021) In the proxy loader used by the nextjs SDK to autowrap user code, we currently write a temporary file to disk for each proxy module we create, because by default Rollup's API expects a path to a file, not the file's contents. It's a little bit of an ugly hack, though, and can cause problems because it writes to the user's source directory rather than to their build directory. This switches to the use of virtual files, using the `@rollup/virtual-plugin` plugin. For unclear reasons, this seems to change (and not in a good way) the base of the relative paths calculated by Rollup when transforming the proxy templates' `import * as wrappedFile from <wrapped file>` and `export * from <wrapped file>` statements. We already have to manually fix those statements, to undo the fact that Rollup substitutes underscores for any square brackets which appear in the paths, but that fix is straightforward because it's very easy to predict what the wrong version will look like, so finding it and overwriting it with the right version is easy. With the relative path base change introduced by the virtual plugin it's not as simple, though, because there's not an easily-discernible pattern to what new base gets picked. (Sometimes it's `pages/`, sometimes it's `pages/api`, sometimes the path uses `./xxx`, sometimes the path uses `../xxx`...) Therefore, rather than trying to nail down and then handle each case of that logic in order to _predict_ what the incorrect path will be, this looks for the transformed version of the whole `import * as wrappedFile from <wrapped file>` statement and reads the incorrect path from there using a regex. Note: This is a second attempt at #5960, which was reverted. H/t to the author of that PR, @lforst, for rubber-ducking this new solution. Fixes #5944.
Resolves #5944
Changes our loader so that we're not writing temporary files to disk anymore and are instead using a virtual file when "rollupizing" (via the
@rollup/plugin-virtual) plugin.How?
So in each template file, we have two imports/exports that grab stuff from the file we want to proxy. The
__RESOURCE_PATH__parts are getting string replaced by absolute paths to the proxied file:import * as wrapee from '__RESOURCE_PATH__';export * from '__RESOURCE_PATH__';We process the template files with rollup because it handily turns the export statement (2) from a wildcard export (which Next.js doesn't allow) into a named export that contains all the actual exports (e.g.
export { A, B, C } from '__RESOURCE_PATH__').We don't need rollup to process the import statement (1) because wildcard imports are allowed in Next.js. For this reason, this PR adds a query string to the import statement (
import * as wrapee from '__RESOURCE_PATH__?__sentry_external__';) telling rollup to ignore this import and to treat it as some external module. This also has the upside that rollup won't mess with the import path by turning it into a relative path that doesn't exist or anything similar. It's just safe.Now we're left with the export statement (2). Rollup will process that statement and turn it into a relative path. That sounds all nice and dandy, however, when using the
@rollup/plugin-virtualplugin rollup messes the relative path up, by pointing it one folder layer upwards -../myPage.jsinstead of./myPage.js- please don't ask why because I also don't know why this is happening.To "fix" the faulty relative path, we do a simple text replacement to turn the double dot
..into a.. The?__sentry_external__query string from before is helpful here because it prevents us from accidentally also doing this replacing for the import statement (1). Additionally, and this was here before, we append a?__sentry_wrapped__so that we're not infinitely wrapping the proxied file.