fix(pnp): make sure that the package locator is fetched with a trailing slash #6882

zregvart · 2019-01-07T15:54:05Z

Summary

I could not get P'n'P working on a project using critical[1] and traced it down to a lookup of a package locator without a trailing slash.

A require('resolve').sync('fg-loadcss') from inline-critical[2] would fail to resolve as the locatorsByLocations lookup in .pnp.js would be using a key without a trailing slash, i.e.:

"../../.cache/yarn/v4/npm-inline-critical-4.0.7-3ffba6a869f39215c8bb00ed2dd6b872e7f98adc/node_modules/inline-critical"

findPackageLocator is invoked with location parameter (notice no trailing slash):

location = "/home/zregvart/.cache/yarn/v4/npm-inline-critical-4.0.7-3ffba6a869f39215c8bb00ed2dd6b872e7f98adc/node_modules/inline-critical"

from resolveToUnqualified with parameters:

request = "fg-loadcss/package.json"
issuer = "/home/zregvart/.cache/yarn/v4/npm-inline-critical-4.0.7-3ffba6a869f39215c8bb00ed2dd6b872e7f98adc/node_modules/inline-critical"

All starting from loadNodeModulesSync in resolve's sync.js[3] that ends up having absoluteStart without the trailing slash.

Not sure if this is the most correct way of fixing this issue, by just ensuring that the trailing slash needed for the lookup is added if missing.

All tests from yarn test and from packages/pkg-tests yarn jest yarn.test.js pass, so it seems like a start.

Test plan

This is how to reproduce:

package.json:

{
  "name": "require-failure",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "scripts": {
    "install": "node --require ./.pnp.js index.js"
  },
  "devDependencies": {
    "critical": "^1.3.4"
  },
  "installConfig": {
    "pnp": true
  }
}

index.js:

require('critical').generate({
  inline: true,
  html: '<!DOCTYPE html><html><head><link rel="stylesheet" href="site.css"></head><body><h1>Hello</h1></body></html>'
});

site.css:

h1 { font-weight: bold; }

Not sure how to create a package test from that example (sorry).

[1] https://github.com/addyosmani/critical
[2] https://github.com/bezoerb/inline-critical/blob/340db21f6a2454ed74ede7a0b317d4c4e177770d/index.js#L32
[3] https://github.com/browserify/resolve/blob/254bb4029df2f8d20a33043dfabd8e5cabfa37df/lib/sync.js#L52

arcanis · 2019-01-07T16:05:08Z

Hey, thanks for investigating! We use the trailing slash to differentiate when the source is a file (/foo/file.js) vs a generic folder (/foo/). I guess always adding a trailing slash would work, but I'd prefer to keep the differentiation unless we don't have the choice.

Can you instead try changing basedir in the following line by basedir.replace(/\/?$/, `/`)?
https://github.com/yarnpkg/yarn/blob/master/src/util/generate-pnp-map-api.tpl.js#L776-L777

... fetched with a trailing slash I could not get P'n'P working on a project using `critical`[1] and traced it down to a lookup of a package locator without a trailing slash. A `require('resolve').sync('fg-loadcss')` from `inline-critical`[2] would fail to resolve as the `locatorsByLocations` lookup in `.pnp.js` would be using a key without a trailing slash, i.e.: ``` "../../.cache/yarn/v4/npm-inline-critical-4.0.7-3ffba6a869f39215c8bb00ed2dd6b872e7f98adc/node_modules/inline-critical" ``` `findPackageLocator` is invoked with `location` parameter (notice no trailing slash): ``` location = "/home/zregvart/.cache/yarn/v4/npm-inline-critical-4.0.7-3ffba6a869f39215c8bb00ed2dd6b872e7f98adc/node_modules/inline-critical" ``` from `resolveToUnqualified` with parameters: ``` request = "fg-loadcss/package.json" issuer = "/home/zregvart/.cache/yarn/v4/npm-inline-critical-4.0.7-3ffba6a869f39215c8bb00ed2dd6b872e7f98adc/node_modules/inline-critical" ``` All starting from `loadNodeModulesSync` in `resolve`'s `sync.js`[3] that ends up having `absoluteStart` without the trailing slash. Not sure if this is the most correct way of fixing this issue, by just ensuring that the trailing slash needed for the lookup is added if missing. All tests from `yarn test` and from `packages/pkg-tests` `yarn jest yarn.test.js` pass, so it seems like a start. This is how to reproduce: package.json: ```json { "name": "require-failure", "version": "1.0.0", "main": "index.js", "license": "MIT", "scripts": { "install": "node --require ./.pnp.js index.js" }, "devDependencies": { "critical": "^1.3.4" }, "installConfig": { "pnp": true } } ``` index.js: ```javascript require('critical').generate({ inline: true, html: '<!DOCTYPE html><html><head><link rel="stylesheet" href="site.css"></head><body><h1>Hello</h1></body></html>' }); ``` site.css: ```css h1 { font-weight: bold; } ``` Not sure how to create a package test from that example (sorry). [1] https://github.com/addyosmani/critical [2] https://github.com/bezoerb/inline-critical/blob/340db21f6a2454ed74ede7a0b317d4c4e177770d/index.js#L32 [3] https://github.com/browserify/resolve/blob/254bb4029df2f8d20a33043dfabd8e5cabfa37df/lib/sync.js#L52

zregvart · 2019-01-07T16:29:39Z

@arcanis thanks for the suggestion, looks much better to my layman eyes at least.

... version We need a patched version of Yarn that includes a fix for properly resolving dependencies in certain cases[1]. [1] yarnpkg/yarn#6882

arcanis · 2019-01-14T15:10:43Z

Thanks! This should be available soon through our nightlies builds 🙂

* master: (67 commits) Include key info for "expected hoisted" invariant (yarnpkg#7009) refactor: remove unnecessary checks (yarnpkg#6955) fix: drive letter casing for win32 pnp (yarnpkg#7007) Don’t call `release` with an exit code (yarnpkg#6981) Check os and platform even when engines is not present in package.json (yarnpkg#6976) Fix handling of non-offline errors (yarnpkg#6968) Treat the ignore-scripts in yarnrc as a synonym to the cli arg (yarnpkg#6983) fix(pnp): make sure pnp module is again the first preloaded module. (yarnpkg#6951) refactor: remove unused imports (yarnpkg#6956) Add 1.14.0 to changelog (yarnpkg#6967) 1.15.0-0 v1.14.0 Fix suggested command after unlinking a package (yarnpkg#6931) Update CHANGELOG.md fix(pnp): make sure pnp module is the first preloaded module. (yarnpkg#6942) fix(pnp): make sure that the package locator is fetched with a trailing slash (yarnpkg#6882) Improve rendering of Chocolatey package description (yarnpkg#6899) Fixing dynamic require missing from webpack (yarnpkg#6908) feat(policies): Use github access token when requesting releases (yarnpkg#6912) Fixes PnP detection across workspaces (yarnpkg#6878) ...

zregvart force-pushed the pr/require-pnp-handling branch from ab84ee6 to ee7bf5d Compare January 7, 2019 16:27

Update CHANGELOG.md

1aeae1e

arcanis merged commit fb03788 into yarnpkg:master Jan 14, 2019

zregvart deleted the pr/require-pnp-handling branch January 15, 2019 01:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(pnp): make sure that the package locator is fetched with a trailing slash #6882

fix(pnp): make sure that the package locator is fetched with a trailing slash #6882

zregvart commented Jan 7, 2019

arcanis commented Jan 7, 2019

zregvart commented Jan 7, 2019

arcanis commented Jan 14, 2019

fix(pnp): make sure that the package locator is fetched with a trailing slash #6882

fix(pnp): make sure that the package locator is fetched with a trailing slash #6882

Conversation

zregvart commented Jan 7, 2019

arcanis commented Jan 7, 2019

zregvart commented Jan 7, 2019

arcanis commented Jan 14, 2019