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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from Jan 14, 2019
Merged

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

merged 2 commits into from Jan 14, 2019

Commits on Jan 7, 2019

  1. fix(pnp): make sure that the package locator is... 

    ... 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
    zregvart committed Jan 7, 2019
    Copy the full SHA
    ee7bf5d View commit details
    Browse the repository at this point in the history

Commits on Jan 14, 2019

  1. Update CHANGELOG.md

    @arcanis
    arcanis committed Jan 14, 2019
    Copy the full SHA
    1aeae1e View commit details
    Browse the repository at this point in the history