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

fix: resolve "browser" field when "exports" is present #59

Merged

Conversation

Boshen
Copy link
Member

@Boshen Boshen commented Jan 24, 2024

closes #58

Failed to resolve path in browser mode for postcss's package.json https://github.com/postcss/postcss/blob/763d57b78a57b7abb6aaf745ab046ad9380cca9c/package.json#L138-L144 when

"exports": {
  ... lots of values
}

"browser": {
    "./lib/terminal-highlight": false,
    "source-map-js": false,
    "path": false,
    "url": false,
    "fs": false
  },

Are both present.

There are no such test cases for this type of combination in enhanced_resolve.

closes #58

Failed to resolve `path` in browser mode for `postcss` when

```
"exports": {
  ...
}

"browser": {
    "./lib/terminal-highlight": false,
    "source-map-js": false,
    "path": false,
    "url": false,
    "fs": false
  },
```

Are both present

There are no such test cases for this type of combination in enhanced_resolve.
Copy link

codspeed-hq bot commented Jan 24, 2024

CodSpeed Performance Report

Merging #59 will not alter performance

⚠️ No base runs were found

Falling back to comparing 01-24-fix_resolve_browser_field_when_exports_is_present (d8a4963) with main (372f078)

Summary

✅ 2 untouched benchmarks

@Boshen Boshen merged commit d7ebd1c into main Jan 24, 2024
18 checks passed
@Boshen Boshen deleted the 01-24-fix_resolve_browser_field_when_exports_is_present branch January 24, 2024 06:16
Comment on lines +2 to +3
"exports": {
},

Choose a reason for hiding this comment

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

You have quoted postcss to be the reason behind the change. And I agree that perhaps in their situation the browser field still should be used (not because I agree with it personally but because this is what webpack and others do). But postcss situation is somewhat different than what you have here in this test case.

I think it would make sense to:

  • do the browser-based resolution after resolving exports for relative import sources
  • always use the browser-based resolution for absolute/bare import sources

I didn't test the latter but I did test the first thing with webpack and I think it behaves like I've described. The in-package files are only resolved through the browser field (aka alias field) after a successful match is found in package.json#exports

Choose a reason for hiding this comment

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

A useful mental model for this is that exports allow you to create arbitrary mappings - there is no real relation between the entry point and the filename. But the alias field is all about rewriting file paths - and that's kinda why it's allowed to be resolved after the actual file path gets resolve based on exports (although you could just encode everything in the exports using a browser condition and that would be way cleaner 🤷‍♂️ )

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

Successfully merging this pull request may close these issues.

Fails to resolve when both "exports" and "browser" field are present
2 participants