Skip to content
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

v1.21.0 breaks with next.js, or without graceful-fs upgrade #264

Closed
benbender opened this issue Jan 5, 2022 · 23 comments
Closed

v1.21.0 breaks with next.js, or without graceful-fs upgrade #264

benbender opened this issue Jan 5, 2022 · 23 comments

Comments

@benbender
Copy link

benbender commented Jan 5, 2022

Sadly, v1.21.0 seems to break babel-plugin-macros. If I pin resolve to v1.20.0, everything is fine. 2.0.0-next.3 also seems to be working. This is especially hard to debug, as its a transient dependency of many libs/plugins and a minor update. It took me quite a while to find it... :)

Related stacktrace:

Uncaught     at Object.statSync (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/next@12.0.0_react-dom@17.0.2+react@17.0.2/node_modules/next/dist/compiled/@vercel/nft/index.js:1:293416)
    at isDirectory (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/resolve@1.21.0/node_modules/resolve/lib/sync.js:22:23)
    at loadNodeModulesSync (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/resolve@1.21.0/node_modules/resolve/lib/sync.js:191:17)
    at Function.resolveSync [as sync] (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/resolve@1.21.0/node_modules/resolve/lib/sync.js:98:17)
    at nodeResolvePath (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/babel-plugin-macros@3.1.0/node_modules/babel-plugin-macros/dist/index.js:62:18)
    at applyMacros (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/babel-plugin-macros@3.1.0/node_modules/babel-plugin-macros/dist/index.js:203:23)
    at ImportDeclaration (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/babel-plugin-macros@3.1.0/node_modules/babel-plugin-macros/dist/index.js:110:28)
    at NodePath._call (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/next@12.0.0_react-dom@17.0.2+react@17.0.2/node_modules/next/dist/compiled/babel/bundle.js:1890:292021)
    at NodePath.call (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/next@12.0.0_react-dom@17.0.2+react@17.0.2/node_modules/next/dist/compiled/babel/bundle.js:1890:291845)
    at NodePath.visit (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/next@12.0.0_react-dom@17.0.2+react@17.0.2/node_modules/next/dist/compiled/babel/bundle.js:1890:292796)
    at processResult (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/next@12.0.0_react-dom@17.0.2+react@17.0.2/node_modules/next/dist/compiled/webpack/bundle5.js:53057:19)
    at <unknown> (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/next@12.0.0_react-dom@17.0.2+react@17.0.2/node_modules/next/dist/compiled/webpack/bundle5.js:53159:5)
    at <unknown> (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/next@12.0.0_react-dom@17.0.2+react@17.0.2/node_modules/next/dist/compiled/webpack/bundle5.js:138518:11)
    at <unknown> (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/next@12.0.0_react-dom@17.0.2+react@17.0.2/node_modules/next/dist/compiled/webpack/bundle5.js:138370:18)
    at context.callback (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/next@12.0.0_react-dom@17.0.2+react@17.0.2/node_modules/next/dist/compiled/webpack/bundle5.js:138243:13)
    at <unknown> (file:///~/next.js/examples/with-babel-macros/node_modules/.pnpm/next@12.0.0_react-dom@17.0.2+react@17.0.2/node_modules/next/dist/build/babel/loader/index.js:33:61)
@tvsbrent
Copy link

tvsbrent commented Jan 5, 2022

Can confirm, we ran into the same issue with version 1.21.0 with a dependency that referenced @babel/core. We had to pin resolve to version 1.20.0 to correct the issue. We think this change in version 1.21.0 is what's causing it:

 - [Refactor] `sync`: Do not throw on missing files in `isFile`/`isDirectory` (#256)

My guess is that libraries were depending on the error being thrown to determine that the file / directory wasn't present.

@ljharb
Copy link
Member

ljharb commented Jan 5, 2022

Oof, ok - any idea which ones?

@tvsbrent
Copy link

tvsbrent commented Jan 5, 2022

In our particular case, the library that was the upstream dependent of @babel/core was the @cypress/code-coverage library. Here's a snippet of the dependency tree:


├─┬ @cypress/code-coverage@3.9.11
│ ├─┬ @cypress/browserify-preprocessor@3.0.1
│ │ ├─┬ @babel/core@7.4.5
| | | ├─ ...
│ │ │ ├─┬ resolve@1.21.0

@ljharb
Copy link
Member

ljharb commented Jan 5, 2022

And it was Babel breaking, or cypress?

@benbender
Copy link
Author

@ljharb It was babel, in my case, babel-plugin-macros. Cypress played no role in my setup (see the stacktrace above).

@tvsbrent
Copy link

tvsbrent commented Jan 5, 2022

This is the full stack from Cypress. We were hacking up code yesterday to understand the issue, and I think we modified graceful-fs/polyfills.js to allow it to handle an undefined response from the stat function.

Oops...we found an error preparing this test file:

  cypress/support/index.js

The error was:

TypeError: Cannot read properties of undefined (reading 'uid') while parsing file: /orm/service/cypress/support/index.js
    at Object.statSync (/root/.cache/Cypress/8.6.0/Cypress/resources/app/packages/server/node_modules/graceful-fs/polyfills.js:303:17)
    at isFile (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/resolve/lib/sync.js:12:23)
    at loadpkg (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/resolve/lib/sync.js:138:14)
    at loadAsFileSync (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/resolve/lib/sync.js:107:19)
    at Function.resolveSync [as sync] (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/resolve/lib/sync.js:93:17)
    at resolveStandardizedName (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/files/plugins.js:101:31)
    at resolvePlugin (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/files/plugins.js:54:10)
    at loadPlugin (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/files/plugins.js:62:20)
    at createDescriptor (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/config-descriptors.js:154:9)
    at /usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/config-descriptors.js:109:50
    at Array.map (<anonymous>:null:null)
    at createDescriptors (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/config-descriptors.js:109:29)
    at createPluginDescriptors (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/config-descriptors.js:105:10)
    at /usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/config-descriptors.js:63:49
    at cachedFunction (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/caching.js:33:19)
    at plugins (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/config-descriptors.js:28:77)
    at mergeChainOpts (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/config-chain.js:319:26)
    at /usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/config-chain.js:283:7
    at buildRootChain (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/config-chain.js:68:29)
    at loadPrivatePartialConfig (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/partial.js:85:55)
    at Object.loadPartialConfig (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/@babel/core/lib/config/partial.js:110:18)
    at transform (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/babelify/index.js:157:17)
    at BabelifyStream._flush (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/babelify/index.js:138:5)
    at BabelifyStream.prefinish (internal/streams/transform.js:147:10)
    at BabelifyStream.emit (events.js:376:20)
    at prefinish (internal/streams/writable.js:630:14)
    at finishMaybe (internal/streams/writable.js:638:5)
    at BabelifyStream.Writable.end (internal/streams/writable.js:582:5)
    at DestroyableTransform.onend (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/readable-stream/lib/_stream_readable.js:577:10)
    at Object.onceWrapper (events.js:482:28)
    at DestroyableTransform.emit (events.js:388:22)
    at endReadableNT (/usr/local/lib/node_modules/@cypress/code-coverage/node_modules/readable-stream/lib/_stream_readable.js:1010:12)
    at processTicksAndRejections (internal/process/task_queues.js:82:21)


This occurred while Cypress was compiling and bundling your test code. This is usually caused by:

- A missing file or dependency
- A syntax error in the file or one of its dependencies

Fix the error in your code and re-run your tests.

@ljharb
Copy link
Member

ljharb commented Jan 5, 2022

@benbender's example doesn't include graceful-fs either, which is helpful to narrow things down. However, it is using pnpm, which complicates matters. @benbender, you said v2 also works - how are you confirming that?

@tvsbrent's example suggests it might be the graceful-fs bug that should be fixed by yesterday evening's release of graceful-fs - can you confirm graceful-fs is fully upgraded?

To be clear, I could just revert the change, but I want to do it along with a test that prevents it from happening again.

@tvsbrent
Copy link

tvsbrent commented Jan 5, 2022

@ljharb Yep, it does appear that using 4.29 of graceful-fs addresses the issue we were seeing.

@benbender
Copy link
Author

benbender commented Jan 5, 2022

@ljharb it's not depending on the package manager - npm, yarn, pnpm have the same issue (confirmed this morning).
You could use https://github.com/vercel/next.js/tree/canary/examples/with-babel-macros as a quite simple reproduction for testing. Simply clone it and run "npm install && npm dev" to see the error in the browser.

About v2 working: I've pinned both version (v1.20 and v2) and both were working for me.

I think it may be the case that the changes in v1.21 are triggering bugs in multiple packages (graceful-fs and @babel/core at least) as I don't think my babel-macro-setup is dependent on graceful-fs afaik.

@ljharb
Copy link
Member

ljharb commented Jan 5, 2022

Thanks, trying to repro now (with npm run dev, since npm dev isn't a thing). Any error on the command line, or only in the browser (which would implicate the bundler)?

@benbender
Copy link
Author

@ljharb you should see TypeError: Cannot read property 'uid' of undefined on the cli and the browser's console. The stacktrace was only visible in the browser. Seeing the error in both cases depends simply on nextjs' ability to do server-side rendering, so no clear indication in my view.

@ljharb
Copy link
Member

ljharb commented Jan 5, 2022

@benbender the stack trace suggests that it's because next/dist/compiled/@vercel/nft/index.js ships with graceful-fs compiled directly into it, so it's lacking the bugfix published 16 hours ago.

It seems like a pretty big concern that next is publishing bundled third-party modules, but it also seems like the quickest fix is for next to update its deps and publish a new release.

@benbender
Copy link
Author

@ljharb I don't think that this is the problem. The problem occurred without an update of nextjs and is not resolved if I pin graceful-fs to v4.2.9... Sorry!

@ljharb
Copy link
Member

ljharb commented Jan 5, 2022

@benbender right, that's what i'm saying. next doesn't require graceful-fs, it bundles it - so you have no ability to alter which version of graceful-fs it's using.

Which means that v1.21.0 of resolve's new usage of throwIfNoEntry is what's triggering the bug in an unfixed graceful-fs that's bundled inside of next itself.

@benbender
Copy link
Author

@ljharb would you be able to open a bug in @vercel/nft as you have the deeper understanding of the problem?

@ljharb
Copy link
Member

ljharb commented Jan 5, 2022

I don’t use it; it’d be great if you filed it, and I’ll comment on it to expand as i can.

@ljharb ljharb changed the title v1.21.0 breaks babel-plugin-macros v1.21.0 breaks with next.js, or without graceful-fs upgrade Jan 5, 2022
@benbender
Copy link
Author

Done in vercel/nft#257

@markjm
Copy link
Contributor

markjm commented Jan 5, 2022

Yikes, @ljharb - looks like my concern about unintended use & monkey patching did come back to bite us 😅. Happy to see graceful-fs produced the requisite change and things are unbroken again.

@ljharb
Copy link
Member

ljharb commented Jan 5, 2022

@benbender looks like the issue needs to be filed on next itself, since that's the package doing the problematic bundling.

kodiakhq bot pushed a commit to vercel/next.js that referenced this issue Jan 6, 2022
This bumps `@vercel/nft` to the latest version and consequently bumps `graceful-fs` to the latest version.

- Fixes #33003
- Related to vercel/nft#258
- Related to browserify/resolve#264

Co-authored-by: JJ Kasper <22380829+ijjk@users.noreply.github.com>
@ljharb
Copy link
Member

ljharb commented Jan 6, 2022

Looks like next@12.0.8-canary.19 may solve this - if someone could try it out and confirm, that'd be great.

@xvvvyz
Copy link

xvvvyz commented Jan 6, 2022

@ljharb upgrading to next@12.0.8-canary.19 did indeed fix this issue for me.

I came across the error by using babel-plugin-inline-react-svg for reference.

@ljharb
Copy link
Member

ljharb commented Jan 6, 2022

Great to hear. I'll close this once v12.0.8 of next is published.

In summary:

  • v1.21 of resolve started using throwIfNoEntry with fs.statSync
  • graceful-fs had a bug with throwIfNoEntry, which was fixed in v4.2.9. (Using resolve v1.2andgraceful-fs` < 4.2.9 in combination is one way to trigger the bug)
  • next prior to v12.0.8 sadly bundles, transitively, a version of graceful-fs earlier than v4.2.9 (if they hadn't bundled this, then users would have been able to fix it themselves. since @vercel/nft switched to not using graceful-fs to monkeypatch the environment, this won't likely be a problem in the future)

@ljharb
Copy link
Member

ljharb commented Jan 14, 2022

next@12.0.8 is released, so I'm going to close this.

Cypress still needs to do a fix (cypress-io/cypress#19610), and meteor-tool (#271), but everything else seems to be fine, and none of this has been an actual problem with resolve.

Thanks for reporting!

@ljharb ljharb closed this as completed Jan 14, 2022
natew pushed a commit to natew/next.js that referenced this issue Feb 16, 2022
This bumps `@vercel/nft` to the latest version and consequently bumps `graceful-fs` to the latest version.

- Fixes vercel#33003
- Related to vercel/nft#258
- Related to browserify/resolve#264

Co-authored-by: JJ Kasper <22380829+ijjk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants