Resolver: Fix browser-spec bare-specifier->relative path redirect for nested/first-party package.jsons#1720
Closed
robhogan wants to merge 1 commit into
Closed
Resolver: Fix browser-spec bare-specifier->relative path redirect for nested/first-party package.jsons#1720robhogan wants to merge 1 commit into
robhogan wants to merge 1 commit into
Conversation
… nested/first-party package.jsons Summary: The [browser spec](https://github.com/defunctzombie/package-browser-field-spec) redirect for bare specifiers (`"browser": {"foo-pkg": "./shims/foo.js"}`) was previously rebased against a path derived by slicing `originModulePath` after the last `node_modules/` segment. That happens to be correct only in the (common) case that the closest `package.json` is the one at the root of a containing `node_modules` package. However: - It's legal for an NPM package to contain nested `package.json` files and those may contain redirects - in which case their targets are relative to themselves, not to the package root `package.json`. - For origin modules that aren't located inside a `node_modules` tree (e.g. project-local source), `lastIndexOf('node_modules/')` returns `-1`, producing a malformed root path and either failed resolutions or surprising successes. This diff replaces that derivation with `closestPackageToOrigin.rootPath`, which we already look up at the top of `resolve`. Behaviour is identical in the already-working cases, and corrected otherwise. Matches webpack's `AliasFieldPlugin`, which rebases against `request.descriptionFileRoot`. Changelog: ``` - **[Fix]**: Fix browser-spec redirects of bare specifier to relative path, where `package.json` is not directly under `node_modules/<pkg>/` ```` Reviewed By: huntie Differential Revision: D105485945
Contributor
|
@robhogan has exported this pull request. If you are a Meta employee, you can view the originating Diff in D105485945. |
Contributor
|
This pull request has been merged in 385a946. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
The browser spec redirect for bare specifiers (
"browser": {"foo-pkg": "./shims/foo.js"}) was previously rebased against a path derived by slicingoriginModulePathafter the lastnode_modules/segment. That happens to be correct only in the (common) case that the closestpackage.jsonis the one at the root of a containingnode_modulespackage. However:It's legal for an NPM package to contain nested
package.jsonfiles and those may contain redirects - in which case their targets are relative to themselves, not to the package rootpackage.json.For origin modules that aren't located inside a
node_modulestree (e.g. project-local source),lastIndexOf('node_modules/')returns-1, producing a malformed root path and either failed resolutions or surprising successes.This diff replaces that derivation with
closestPackageToOrigin.rootPath, which we already look up at the top ofresolve. Behaviour is identical in the already-working cases, and corrected otherwise. Matches webpack'sAliasFieldPlugin, which rebases againstrequest.descriptionFileRoot.Changelog:
Reviewed By: huntie
Differential Revision: D105485945