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

CommonJS import runtime issue since 0.14.21 #2239

Closed
martpie opened this issue May 9, 2022 · 3 comments
Closed

CommonJS import runtime issue since 0.14.21 #2239

martpie opened this issue May 9, 2022 · 3 comments

Comments

@martpie
Copy link

martpie commented May 9, 2022

Hello 👋

Before I start, I would like to thank you for this amazing tool that ESBuild is, and more specifically the releases notes, that are always extremely educational and interesting to read, I love them!

So, I've been trying to debug a very weird issue that happens since I updated my version of ESBuild from 0.14.8 to 0.14.38. I've been able to pinpoint the faulty release: 0.14.21 (0.14.20 is working fine).

I am working with some old packages (I cannot upgrade this dependency as it's "consumer" is not really maintained anymore). I suspect the issue lies on the dependency package.json and how the module is resolved.

I am very unsure of what is going on, so if I can help providing additional information, or if some of the information below is irrelevant, please tell me!

So, the issue:

  • level-js@4.0.2
  • Error: "Uncaught TypeError: setImmediate2 is not a function"

Stack trace (going through the bundle):

Screenshot 2022-05-09 at 10 55 29

It would seem setImmediate2 is an empty object here, and not a function.

Screenshot 2022-05-09 at 10 56 58

Screenshot 2022-05-09 at 10 57 45

So from here, we can see an import is trying to import itself? Which looks fishy to me.

Reproduction steps:

  • git clone https://github.com/martpie/museeks.git
  • cd museeks
  • yarn && yarn dev
  • In another terminal: yarn museeks:debug (you may need to click on the app icon in your dock, as the logic is to show the window only when the app initiates successfully)
  • On the view menu, click on "Toggle Developer Tools" if they're not already open

If there's anything I can do to help debug this issue, please tell me! :)

@hyrious
Copy link

hyrious commented May 9, 2022

Running the build script with logLevel: 'verbose' will give us the reason:

⬥ [VERBOSE] Resolving import "immediate" in directory "/Users/hyrious/pg/museeks/node_modules/level-js/util" of type "require-call"

  Checking "immediate" against the external pattern "electron/*"
  Checking "immediate" against the external pattern "fs/*"
  Checking "immediate" against the external pattern "stream/*"
  Checking "immediate" against the external pattern "path/*"
  Checking "immediate" against the external pattern "platform/*"
  Checking "immediate" against the external pattern "assert/*"
  Checking "immediate" against the external pattern "os/*"
  Checking "immediate" against the external pattern "constants/*"
  Checking "immediate" against the external pattern "util/*"
  Checking "immediate" against the external pattern "events/*"
  Read 4 entries for directory "/Users/hyrious/pg/museeks/node_modules/level-js/util"
  Checking for "immediate" in the "browser" map in "/Users/hyrious/pg/museeks/node_modules/level-js/package.json"
    Checking for "immediate"
    Checking for "immediate.tsx"
    Checking for "immediate.ts"
    Checking for "immediate.jsx"
    Checking for "immediate.js"
    Checking for "immediate.css"
    Checking for "immediate.json"
    Checking for "immediate/index"
    Checking for "immediate/index.tsx"
    Checking for "immediate/index.ts"
    Checking for "immediate/index.jsx"
    Checking for "immediate/index.js"
    Checking for "immediate/index.css"
    Checking for "immediate/index.json"
  Checking for "./util/immediate" in the "browser" map in "/Users/hyrious/pg/museeks/node_modules/level-js/package.json"
    Checking for "./util/immediate"
    Checking for "./util/immediate.tsx"
    Checking for "./util/immediate.ts"
    Checking for "./util/immediate.jsx"
    Checking for "./util/immediate.js"
  Found "./util/immediate.js" mapping to "./util/immediate-browser.js"
  Attempting to load "/Users/hyrious/pg/museeks/node_modules/level-js/util/immediate-browser.js" as a file
    Checking for file "immediate-browser.js"
    Found file "immediate-browser.js"
  Read 4 entries for directory "/Users/hyrious/pg/museeks/node_modules/level-js/util"
  Ignoring "/Users/netless/hyrious/pg/museeks/tsconfig.json" because "/Users/hyrious/pg/museeks/node_modules/level-js/util/immediate-browser.js" is inside "node_modules"
  Primary path is "/Users/hyrious/pg/museeks/node_modules/level-js/util/immediate-browser.js" in namespace "file"

It turns out that require('immediate') in level-js/util/immediate-browser.js is resolved to the same file, because it searches ./util/immediate in the browser field and successfully found the same file. Looking into the CHANGELOG we can find that this case (resolving names without './' from browser field) was added for replicating Browserify's bug.

@martpie
Copy link
Author

martpie commented May 9, 2022

Thank you very much for the hints!

we can find that this case was added for replicating Browserify's bug.

So in theory, who should be "responsible" for solving it? ESBuild, level-js, or myself by tweaking some module resolutions?

@evanw
Copy link
Owner

evanw commented May 11, 2022

Well ideally Browserify could just fix their bug: browserify/browserify#1994. I don't expect that to happen, however. They seem to be uninterested in fixing it. The problem is that (as far as I can tell) Browserify invented the browser field and didn't specify how it should work, so the specification is sort of "however Browserify does it" which means reproducing their bugs. The approach I've taken with esbuild is "if the mapping applies in any major bundler than it should apply in esbuild" which includes Browserify but also Webpack, Parcel, and Rollup.

I think in this particular case the fix should come from esbuild. I have attempted to reverse-engineer a specification by collecting a set of test cases here: https://github.com/evanw/package-json-browser-tests. It looks like this case you reported isn't represented in that test suite, and adding it does cause esbuild to fail (i.e. to fail when Browserify succeeds). I'll work on a fix.

@evanw evanw closed this as completed in bb7a0a1 May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants