Skip to content

Commit

Permalink
fix(pnp): make sure that the package locator is...
Browse files Browse the repository at this point in the history 
... 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
  • Loading branch information
@zregvart
zregvart committed Jan 7, 2019
1 parent 6c41eb1 commit ab84ee6
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/util/generate-pnp-map.js
View file
Expand Up @@ -107,7 +107,7 @@ function generateFindPackageLocator(packageInformationStores: PackageInformation
code += ` if (!relativeLocation.match(isStrictRegExp))\n`;
code += ` relativeLocation = \`./\${relativeLocation}\`;\n`;
code += `\n`;
code += ` if (location.match(isDirRegExp) && relativeLocation.charAt(relativeLocation.length - 1) !== '/')\n`;
code += ` if (relativeLocation.charAt(relativeLocation.length - 1) !== '/')\n`;
code += ` relativeLocation = \`\${relativeLocation}/\`;\n`;
code += `\n`;
code += ` let match;\n`;
Expand Down

0 comments on commit ab84ee6

Please sign in to comment.