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.

Sign up for GitHub

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

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

Merged
merged 15 commits into from
Jul 11, 2024
Merged

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

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
b8174ce
fix: Handle the scenario where a package.json#browser field could be …
jeffsee55 Jun 21, 2024
a68c80a
Update src/resolve-dependency.ts
jeffsee55 Jun 21, 2024
30fcf60
Remove extra test fixture
jeffsee55 Jun 21, 2024
103aac3
Bump after editing pr title
jeffsee55 Jun 21, 2024
d36adc4
Add test where browser setting is undefined
jeffsee55 Jun 24, 2024
416fca3
Cleanup
jeffsee55 Jun 24, 2024
5f5e7f6
Debug windows-specific issue
jeffsee55 Jun 28, 2024
4e3ece6
Undo windows debug
jeffsee55 Jun 28, 2024
154b203
Bump
jeffsee55 Jul 1, 2024
d225ebb
Merge branch 'main' into jeffsee55/handle-browser-false
jeffsee55 Jul 3, 2024
c201632
Add test for when the browser field is a string
jeffsee55 Jul 5, 2024
18927d8
Undo test fixture change
jeffsee55 Jul 5, 2024
1d48d95
Change browser string test to a string value
jeffsee55 Jul 9, 2024
4195963
Fix unit test
jeffsee55 Jul 9, 2024
59758c2
Merge branch 'main' into jeffsee55/handle-browser-false
kodiakhq[bot] Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/resolve-dependency.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ interface PkgCfg {
main: string | undefined;
exports: PackageTarget;
imports: { [key: string]: PackageTarget };
browser?: string | { [key: string]: string };
browser?: unknown;
}

async function getPkgCfg(
Expand Down Expand Up @@ -259,8 +259,18 @@ async function resolveRemappings(
): Promise<void> {
if (job.conditions?.includes('browser')) {
const { browser: pkgBrowser } = pkgCfg;
if (!pkgBrowser) {
return;
}
Copy link
Member

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

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

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

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

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.

if (typeof pkgBrowser === 'object') {
for (const [key, value] of Object.entries(pkgBrowser)) {
if (typeof value !== 'string') {
/**
* `false` can be used to specify that a file is not meant to be included.
* Downstream processing is expected to handle this case, and it should remain in the mapping result
*/
continue;
}
if (!key.startsWith('./') || !value.startsWith('./')) {
continue;
}
Expand Down
2 changes: 2 additions & 0 deletions test/unit/browser-remappings-false/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# include node_modules for testing
!node_modules
2 changes: 2 additions & 0 deletions test/unit/browser-remappings-false/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
require('pkg');

1 change: 1 addition & 0 deletions test/unit/browser-remappings-false/node_modules/pkg/browser.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions test/unit/browser-remappings-false/node_modules/pkg/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions test/unit/browser-remappings-false/node_modules/pkg/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions test/unit/browser-remappings-false/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[
"package.json",
"test/unit/browser-remappings-false/input.js",
"test/unit/browser-remappings-false/node_modules/pkg/browser.js",
"test/unit/browser-remappings-false/node_modules/pkg/index.js",
"test/unit/browser-remappings-false/node_modules/pkg/package.json"
]
3 changes: 3 additions & 0 deletions test/unit/browser-remappings-false/test-opts.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"conditions": ["browser"]
}
2 changes: 2 additions & 0 deletions test/unit/browser-remappings-undefined/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# include node_modules for testing
!node_modules
2 changes: 2 additions & 0 deletions test/unit/browser-remappings-undefined/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
require('pkg');

1 change: 1 addition & 0 deletions test/unit/browser-remappings-undefined/node_modules/pkg/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions test/unit/browser-remappings-undefined/node_modules/pkg/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions test/unit/browser-remappings-undefined/node_modules/pkg/require-main.cjs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/unit/browser-remappings-undefined/node_modules/pkg/subdir/import-main-browser.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/unit/browser-remappings-undefined/node_modules/pkg/subdir/import-main.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/unit/browser-remappings-undefined/node_modules/pkg/subdir/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions test/unit/browser-remappings-undefined/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[
"package.json",
"test/unit/browser-remappings-undefined/input.js",
"test/unit/browser-remappings-undefined/node_modules/pkg/index.js",
"test/unit/browser-remappings-undefined/node_modules/pkg/package.json",
"test/unit/browser-remappings-undefined/node_modules/pkg/require-main.cjs",
"test/unit/browser-remappings-undefined/node_modules/pkg/subdir/import-main.js",
"test/unit/browser-remappings-undefined/node_modules/pkg/subdir/package.json"
]
3 changes: 3 additions & 0 deletions test/unit/browser-remappings-undefined/test-opts.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"conditions": ["browser"]
}
2 changes: 1 addition & 1 deletion test/unit/browser-remappings/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@
"test/unit/browser-remappings/node_modules/pkg/subdir/import-main-browser.js",
"test/unit/browser-remappings/node_modules/pkg/subdir/import-main.js",
"test/unit/browser-remappings/node_modules/pkg/subdir/package.json"
]
]
Loading