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 ee7bf5d
Showing 1 changed file with 4 additions and 0 deletions.
4 changes: 4 additions & 0 deletions src/util/generate-pnp-map-api.tpl.js
View file
Expand Up @@ -773,6 +773,10 @@ exports.setupCompatibilityLayer = () => {
// Extract the name of the package being requested (1=full name, 2=scope name, 3=local name)
const parts = request.match(/^((?:(@[^\/]+)\/)?([^\/]+))/);

// make sure that basedir ends with a slash
if (basedir.charAt(basedir.length - 1) !== '/') {
basedir = path.join(basedir, '/');
}
// This is guaranteed to return the path to the "package.json" file from the given package
const manifestPath = exports.resolveToUnqualified(`${parts[1]}/package.json`, basedir);

Expand Down

0 comments on commit ee7bf5d

Please sign in to comment.