Skip to content

Find approach that doesn't write to disk in Next.js SDK proxyLoader #5944

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
AbhiPrasad opened this issue Oct 13, 2022 Discussed in #5943 · 7 comments · Fixed by #5960 or #6021
Closed

Find approach that doesn't write to disk in Next.js SDK proxyLoader #5944

AbhiPrasad opened this issue Oct 13, 2022 Discussed in #5943 · 7 comments · Fixed by #5960 or #6021
Assignees
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK

Comments

@AbhiPrasad
Copy link
Member

Discussed in #5943

Originally posted by eggplants October 13, 2022
proxyLoader creates src/pages/temp***.js. It causes the following error in my nextjs project on Docker:

ModuleBuildError: Module build failed (from ./node_modules/@sentry/nextjs/cjs/config/loaders/proxyLoader.js):
Error: EROFS: read-only file system, open '/app/src/pages/temp0.5575603336148998.js'
    at Object.openSync (node:fs:585:3)
    at Object.writeFileSync (node:fs:2170:35)
    at Object.proxyLoader (/app/node_modules/@sentry/nextjs/cjs/config/loaders/proxyLoader.js:68:17)

Current only workaround is to set autoInstrumentServerFunctions: false. Is there any other way?

(@sentry/nextjs: 7.15.0, next: 12.3.1)

@AbhiPrasad AbhiPrasad added Type: Bug Package: nextjs Issues related to the Sentry Nextjs SDK labels Oct 13, 2022
@AbhiPrasad
Copy link
Member Author

AbhiPrasad commented Oct 13, 2022

@eggplants Could you provide your Node/Webpack versions as well here?

I wonder why /app/src/pages is readonly, is this something you configured with docker?

Currently there is no other workaround for this other than disabling autoInstrumentServerFunctions, but we are going to take a look!

Perhaps we need to investigate using a virtual plugin, or using the /tmp/ directory here.

@eggplants
Copy link

eggplants commented Oct 13, 2022

$ node --version
v16.13.0

I wonder why /app/src/pages is readonly, is this something you configured with docker?

Yes. Here is a part of my docker-compose.yaml.

version: '3.6'

services:
...
  nextjs:
    build:
      context: ./
      dockerfile: ./Dockerfile
    image: foo-bar-nextjs
    volumes:
      - ./src:/app/src:ro
 ...

@eggplants
Copy link

eggplants commented Oct 13, 2022

To set autoInstrumentServerFunctions to true while keeping the volume read-only, is new option like disableProxyLoader needed?

@AbhiPrasad
Copy link
Member Author

autoInstrumentServerFunctions uses the proxy loader, so if the proxy loader was not installed it would not do anything.

@eggplants
Copy link

So I have to either give up on making volume read-only or using autoInstrumentServerFunctions, right?

@AbhiPrasad
Copy link
Member Author

Yes for now, but we can look at improving this. PRs are also welcome if you have any ideas.

@eggplants
Copy link

If we can get the rollup to do it without temporary files, this issue might solve the problem.

// Inject the route into the template
templateCode = templateCode.replace(/__ROUTE__/g, parameterizedRoute);
// Fill in the path to the file we're wrapping and save the result as a temporary file in the same folder (so that
// relative imports and exports are calculated correctly).
//
// TODO: We're saving the filled-in template to disk, however temporarily, because Rollup expects a path to a code
// file, not code itself. There is a rollup plugin which can fake this (`@rollup/plugin-virtual`) but the virtual file
// seems to be inside of a virtual directory (in other words, one level down from where you'd expect it) and that
// messes up relative imports and exports. Presumably there's a way to make it work, though, and if we can, it would
// be cleaner than having to first write and then delete a temporary file each time we run this loader.
templateCode = templateCode.replace(/__RESOURCE_PATH__/g, this.resourcePath);
const tempFilePath = path.resolve(path.dirname(this.resourcePath), `temp${Math.random()}.js`);
fs.writeFileSync(tempFilePath, templateCode);
// Run the proxy module code through Rollup, in order to split the `export * from '<wrapped file>'` out into
// individual exports (which nextjs seems to require), then delete the tempoary file.
let proxyCode = await rollupize(tempFilePath, this.resourcePath);
fs.unlinkSync(tempFilePath);

@lforst lforst changed the title Disable proxyLoader Find approach that doesn't write to disk in Next.js SDK proxyLoader Oct 13, 2022
lobsterkatie added a commit that referenced this issue Oct 25, 2022
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK
Projects
None yet
3 participants