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

browser entry point #1160

Closed
wants to merge 1 commit into from
Closed

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Mar 28, 2024

Re: #1156 (comment).

Instead of taking at face value what pathResolution returns, we find package.json then see if the "browser" condition  entry point is present and update pathResolution as a consequence. The spec indicates that there are two possibilities to cover:

  • a string (found in e.g. "supports-color@7.2.0"),
  • a replacement map (found in e.g. "asap@2.0.6").

My tests use two modules that are already here by "chance", and will break if any of these modules gets updated (for example supports-color@9 does not use the browser field anymore), but we can fix that when it happens. (Alternatively, we could add these precise versions in devDependencies just for these tests?)

I checked what @rollup/plugin-node-resolve plugin does: it looks at the entry with the condition key in resolvePackageTarget. The chain of calls: < resolvePackageImportsExports < resolvePackageImports < resolveImportSpecifiers < resolveImportSpecifiers

@Fil Fil requested a review from mbostock March 28, 2024 07:40
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

This is using the browser entry point (or “main field”) which is different from the browser condition within the exports object, which is what I was referring to. The latter is the modern standard that is the top priority to support. I suppose we should also support the browser field too as a fallback (and maybe jsdelivr and unpkg fields, like d3-require does?). It sure is complicated! At least there are modern standards for this, but it’ll be a bit of a mess to try to make everything work.

@Fil Fil changed the title browser condition browser entry point Mar 28, 2024
@mbostock
Copy link
Member

Also I wanted to see if there is a way to use Rollup’s node-resolve plugin to figure some of this stuff out for us. I think it might be possible, but I’m not sure. Anyway I appreciate your help cracking this nut!

@Fil
Copy link
Contributor Author

Fil commented Mar 28, 2024

yup, I also feel this is something that should be left to rollup.

@mbostock
Copy link
Member

Superseded by 50a3e7d.

@mbostock mbostock closed this Mar 28, 2024
@mbostock
Copy link
Member

(I’ll work on porting the tests now.)

@Fil Fil deleted the fil/node-modules-browser branch March 28, 2024 15:05
@mbostock
Copy link
Member

Hmm, I don’t have a good way of porting the tests because we no longer return the resolved path to the entry point — we just pass the entry name to rollup and let it figure it out. So the default entry name is . which we rename to index.js (because we need the .js extension, and I’m assuming no one would need both . and index.js to refer to different things). We’d have to test the transpiled contents, I think. Maybe we need to hand-craft some packages to test.

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.

None yet

2 participants