Skip to content

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

Merged
kodiakhq[bot] merged 15 commits intomainfrom
jeffsee55/handle-browser-false
Jul 11, 2024
Merged

fix: Handle the scenario where a package.json#browser field could be false#427
kodiakhq[bot] merged 15 commits intomainfrom
jeffsee55/handle-browser-false

Conversation

@jeffsee55
Copy link
Contributor

@jeffsee55 jeffsee55 commented Jun 21, 2024 •
edited
Loading

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.

@jeffsee55 jeffsee55 requested review from ijjk and styfle as code owners June 21, 2024 21:32
@jeffsee55 jeffsee55 marked this pull request as draft June 21, 2024 21:32
@jeffsee55 jeffsee55 marked this pull request as ready for review June 21, 2024 21:34
@jeffsee55 jeffsee55 marked this pull request as draft June 21, 2024 21:34
@jeffsee55 jeffsee55 force-pushed the jeffsee55/handle-browser-false branch from 593191b to b8174ce Compare June 21, 2024 21:35
@jeffsee55 @styfle
Update src/resolve-dependency.ts
a68c80a 
Co-authored-by: Steven <steven@ceriously.com>
styfle
styfle reviewed Jun 21, 2024
View reviewed changes
src/resolve-dependency.ts
const { browser: pkgBrowser } = pkgCfg;
if (!pkgBrowser) {
return;
}
Copy link
Member

@styfle styfle Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

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

Copy link
Contributor Author

@jeffsee55 jeffsee55 Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@styfle styfle Jun 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?)

Copy link
Contributor Author

@jeffsee55 jeffsee55 Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dvoytenko dvoytenko Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jeffsee55 changed the title Handle the scenario where a package.json#browser field could be set t… fix: Handle the scenario where a package.json#browser field could be set t… Jun 21, 2024
@TooTallNate TooTallNate changed the title fix: Handle the scenario where a package.json#browser field could be set t… fix: Handle the scenario where a package.json#browser field could be false Jun 21, 2024
@jeffsee55
Copy link
Contributor Author

jeffsee55 commented Jun 24, 2024

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
Copy link
Contributor Author

jeffsee55 commented Jul 2, 2024

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

#432

@jeffsee55 jeffsee55 marked this pull request as ready for review July 3, 2024 20:07
@styfle styfle requested a review from a team July 3, 2024 21:30
styfle
styfle reviewed Jul 3, 2024
View reviewed changes
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jeffsee55
Copy link
Contributor Author

jeffsee55 commented Jul 3, 2024

@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
Copy link
Member

styfle commented Jul 3, 2024

@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"

@jeffsee55 jeffsee55 added the automerge Automatically merge PR once checks pass label Jul 11, 2024
@kodiakhq kodiakhq bot merged commit 099608f into main Jul 11, 2024
@kodiakhq kodiakhq bot deleted the jeffsee55/handle-browser-false branch July 11, 2024 17:48
@github-actions
Copy link

github-actions bot commented Jul 11, 2024

🎉 This PR is included in version 0.27.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jeffsee55 jeffsee55 mentioned this pull request Jul 11, 2024
Merged
kodiakhq bot pushed a commit to vercel/vercel that referenced this pull request Jul 15, 2024
@jeffsee55
Upgrade to @vercel/nft 0.27.3 with a bug fix for browser mapping supp…
11e2274 
…ort (#11841)

See [nft](vercel/nft#427) for details
@leonardoadame leonardoadame mentioned this pull request Sep 11, 2024
Open
This was referenced Sep 19, 2024
Open
Open
Open
Open
@Thompson1985 Thompson1985 mentioned this pull request Oct 7, 2024
Open
@leonardoadame leonardoadame mentioned this pull request Nov 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@styfle styfle styfle left review comments

@TooTallNate TooTallNate TooTallNate approved these changes

@ijjk ijjk Awaiting requested review from ijjk ijjk is a code owner

@dvoytenko dvoytenko Awaiting requested review from dvoytenko

Assignees

No one assigned

Labels

automerge Automatically merge PR once checks pass released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jeffsee55 @styfle @TooTallNate @dvoytenko