fix: Handle the scenario where a package.json#browser field could be false

[jeffsee55]

We recently shipped support for the browser field, but we discovered an edge case where instead of mapping to another string, the value could be false (eg, object-inspect), which indicates that this module should be treated as an empty object ({}) (evidenced by its usage here).

The code was previously erroring on the string's startsWith checks, causing the entire package to be omitted from the file mapping result. This ensures that if the value is false we'll continue on.

[styfle]

Actually now that I think about it, isn't there a case where browser can be a string?

We should add a test for this case as well
https://gist.github.com/defunctzombie/4339901/49493836fb873ddaa4b8a7aa0ef2352119f69211

[jeffsee55]

It can be, but I saw this test and now I think that maybe we should update its implementation (and rename). It's ensuring that the field specified in the browser field is not in the output, but it's not imported anywhere in the other modules so I think it's a false positive (as you pointed out when we were pairing).

[styfle]

@jeffsee55 That is very strange.

Maybe talk to @dvoytenko to see if that is a mistake since I would expect to see import-main-browser.js in the output.js.

Also, that test seems very oddly written since pkg is its own package boundary and pkg/subdir is another package boundary, but the browser field is referencing files from one to the other. I don't think thats common (or even allowed?)

[jeffsee55]

Thanks @styfle , will follow up with Dima.

For posterity, here are my thoughts on the current state of these tests, it seems like the output.js should expect only files that are traceable from nft, but in the tests the browser-specific files aren't imported or required by the paths the relate to the input entrypoint. So in my test I required it specifically. So it seemed like the other tests aren't asserting the right behavior because those browser files just aren't part of the dependency graph.

[dvoytenko]

I think it could be a string, as in "main export file for browser". TBH, I haven't seen those in the wild and not what I've been trying to support in one of my previous PRs. But I think it's a use case in its own right and probably should be supported too. It's a relatively under-defined and legacy area - most of folk moved away from browser, but I found the best way to debug them is by trying out to build examples using esbuild - it understands browser correctly.

[jeffsee55]

This one is failing on Windows only:

2024-06-24T23:06:19.1227177Z 
2024-06-24T23:06:19.1227722Z   ● should correctly trace zeromq-node-gyp from cwd
2024-06-24T23:06:19.1228234Z 
2024-06-24T23:06:19.1228583Z     expect(received).toEqual(expected) // deep equality
2024-06-24T23:06:19.1229108Z 
2024-06-24T23:06:19.1229352Z     - Expected  - 0
2024-06-24T23:06:19.1229760Z     + Received  + 1
2024-06-24T23:06:19.1230017Z 
2024-06-24T23:06:19.1230244Z     @@ -1,7 +1,8 @@
2024-06-24T23:06:19.1231259Z       Array [
2024-06-24T23:06:19.1232608Z         "node_modules\\@aminya\\node-gyp-build\\index.js",
2024-06-24T23:06:19.1234088Z     +   "node_modules\\@aminya\\node-gyp-build\\node-gyp-build.js",
2024-06-24T23:06:19.1235188Z         "node_modules\\@aminya\\node-gyp-build\\package.json",
2024-06-24T23:06:19.1236451Z         "node_modules\\zeromq\\lib\\draft.js",
2024-06-24T23:06:19.1237453Z         "node_modules\\zeromq\\lib\\index.js",
2024-06-24T23:06:19.1238165Z         "node_modules\\zeromq\\lib\\native.js",
2024-06-24T23:06:19.1240146Z         "node_modules\\zeromq\\lib\\util.js",

[jeffsee55]

Confirmed this has nothing to do with this PR, tests are failing here too

#432

[styfle]

Is there a test for a string instead of object? For example, "browser": "./foo.js" ?

[jeffsee55]

@styfle there was already one here https://github.com/vercel/nft/blob/main/test/unit/browser-remappings-malformed/node_modules/pkg/package.json.

However I was hoping to revisit this after we land this to ensure that these tests are actually asserting what we think they are. That test, for example doesn't actually require the file mentioned in the browser setting in the entry. So improving on the tests feels a bit out of my depths

[styfle]

@jeffsee55 I'm not suggesting that you change the existing test. I want a new test that tests for browser as a string.

Something like:

{
  "name": "pkg",
  "main": "index.js",
  "browser": {
    "./browser.js": "browser.js"
  }
}

And then require('pkg') should emit node_modules/pkg/browser.js even though that file isn't explicitly required.

That would prove that your comment is correct when it says "Downstream processing is expected to handle this case, and it should remain in the mapping result"

[github-actions]

🎉 This PR is included in version 0.27.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀