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.

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

Adds support for PnP #2942

Closed
wants to merge 11 commits into from
Closed

Adds support for PnP #2942

wants to merge 11 commits into from

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Apr 22, 2019

↪️ Pull Request

This PR adds support for PnP to the Parcel resolver.

Related: #2635

💻 Examples

Parcel running under Berry now works 💃

🚨 Test instructions

Unit tests usually are a bit impractical since it requires to run under a different environment (since it relies on process.versions.pnp). Any idea? I tested it locally by following the steps in the getting started (absolutely great experience btw!) and adding a require('lodash') in index.js.

To reproduce:

  • Make a new project with Yarn and cd' into it
  • Run yarn policies set-version berry
  • Run yarn --version to confirm you're running with the v2
  • Run yarn add lodash and add the require('lodash') to index.js
  • Run yarn parcel index.html

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Apr 22, 2019

Is there a way you could add a test for this?

Seems rather complex to test but would be nice if we have a test for it

Sent with GitHawk

@arcanis
Copy link
Contributor Author

arcanis commented Apr 22, 2019

Just pushed a commit with a test. I cheat a bit by modifying (then restoring) the environment but it should be fine as long as the tests don't run in parallel 🤞

I also had to commit the result of yarn install && yarn format, as they were changing other files than the ones affected by my PR.

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Super excited about this.

@@ -60,7 +60,17 @@ class Resolver {
let module = await this.resolveModule(filename, parent);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to adjust this slightly so resolveModule is not called when using pnp, since that will still look for a node_modules directory (it calls findNodeModulePath). We'll need to split out the rest of that method so that things like aliases and builtins are still applied, but it doesn't search the filesystem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, resolveModule could also check for pnp and skip looking for node_modules if it is available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, done 👍

@devongovett
Copy link
Member

Does this also support pnp in yarn v1? Should it?

@arcanis
Copy link
Contributor Author

arcanis commented Apr 22, 2019

@devongovett I haven't tested but it should, yes. The pnpapi package is exactly the same from the consumer perspective - only the internal implementation changes a bit.

Of course the v2 is so much better anyway 🤡

@arcanis
Copy link
Contributor Author

arcanis commented May 1, 2019

Any blocker?

DeMoorJasper
DeMoorJasper previously approved these changes May 1, 2019
@arcanis
Copy link
Contributor Author

arcanis commented May 21, 2019

Ping ? 😊

@mischnic
Copy link
Member

@arcanis There is a lint error:

/Users/vsts/agent/2.150.3/work/1/s/packages/core/integration-tests/test/pnp.js
  2:7  error  'fs' is assigned a value but never used  no-unused-vars

@arcanis
Copy link
Contributor Author

arcanis commented Jun 25, 2019

Ping @devongovett ? 🙂

Copy link
Member

@DeMoorJasper DeMoorJasper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, although I don't think Parcel 1 should change this significantly and this should probably target Parcel 2

@texastoland
Copy link

I'm actively waiting on this to support some courses on egghead.io. Is there are part of the change that's too drastic?

@codyaverett
Copy link

This would be nice to have now. Are you all holding off on this for v2?

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Aug 12, 2019

Yes, this PR should be implemented for v2. Alpha's gonna come out soon and Parcel 1 should only receive bugfixes at this point.

It could be a seperate resolver plugin in Parcel 2

@DeMoorJasper
Copy link
Member

closing in favor of #3582

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants