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

Add support for Yarn v2 (PnP) #194

Closed
FredKSchott opened this issue Feb 7, 2020 · 19 comments
Closed

Add support for Yarn v2 (PnP) #194

FredKSchott opened this issue Feb 7, 2020 · 19 comments
Assignees

Comments

@FredKSchott
Copy link
Owner

Original Discussion: https://www.pika.dev/npm/snowpack/discuss/69
/cc @frbrz, @FredKSchott

We already rely on require.resolve() internally, which should mean that Yarn PnP "just works". But, we'll need to confirm. I'd love someones help just trying this out with Yarn PnP. The first step would be to remove these lines from the start:

  if (!fs.existsSync(path.join(cwd, 'node_modules'))) {
    logError('no "node_modules" directory exists. Did you run "npm install" first?');
    return;
  }

And then seeing what happens / if anything else breaks.

@FredKSchott FredKSchott added contributors welcome! contributors welcome! good first issue Good for newcomers labels Feb 7, 2020
@Manish-Giri
Copy link

Hello! I'd like to work on this.

@FredKSchott
Copy link
Owner Author

Awesome! Would love your help on this. As the OP says, the first thing to do is remove the explicit check that node_modules exists, and then see what breaks. We're pretty good about using require.resolve() wherever possible, so there's a slight chance that everything works on our side.

I don't think Rollup will work out of the box with PnP. It looks like we may need to add https://github.com/arcanis/rollup-plugin-pnp-resolve to our plugin chain to solve, in which case we should only do that if we detected PnP mode, vs. always adding this plugin. You should look into how to detect PnP mode (I bet they intentionally made this easy, maybe via process.env.*) vs. vanilla Node.js.

@FredKSchott FredKSchott removed the contributors welcome! contributors welcome! label Feb 25, 2020
@Manish-Giri
Copy link

@FredKSchott Thank You for the directions! I'll get started with this...

@Glazy
Copy link
Contributor

Glazy commented Apr 4, 2020

Hey @FredKSchott 👋

I ran through this locally with the following steps:

  1. Ensure yarn --version is greater than 2.
  2. yarn init and install preact, htm, ramda.
  3. Write some code.
  4. Remove lines mentioned in OP from local snowpack.
  5. Build and run local snowpack.
  6. npx servor --reload
  7. Success 🎉

Stepped through the above and everything seems to work without any changes to the Rollup plugin configuration 👍

EDIT: Just realised this morning that my install script contained --source pika so it doesn't work as expected. That's what I get for coding at 2am 😂

@FredKSchott
Copy link
Owner Author

No way! That's great! So I guess the question then is: how can we detect if a missing node_modules directory is good (user has yarn PnP enabled) vs. bad (user using yarn v1, or npm)?

(Alternatively, we could just wait for the v2 work in #236 where node_modules may no longer be required all the time)

@FredKSchott FredKSchott removed the good first issue Good for newcomers label Apr 4, 2020
@Glazy
Copy link
Contributor

Glazy commented Apr 4, 2020

Had a look into this and we can use process.versions.pnp which would allow us to grab the version of the PnP standard in use (if any)!

@FredKSchott
Copy link
Owner Author

Awesome! Sounds like a quick fix to add that check to the conditional mentioned in OP, Glazy want to tackle in a PR?

@crubier
Copy link

crubier commented Oct 10, 2020

Not sure if I am doing something wrong, but Create Snowpack App with react + typescript does not work with yarn pnp in dev mode. Build is ok, but dev fails with:

[snowpack] installing dependencies...
[snowpack] .yarn/$$virtual/react-three-fiber-virtual-d6aeaf13a8/0/cache/react-three-fiber-npm-5.0.3-96813d739b-99a1d49199.zip/node_modules/react-three-fiber/web.js
Module "object-assign" could not be resolved by Snowpack (Is it installed?).
... followed by a TON of similar errors...

@crubier
Copy link

crubier commented Oct 10, 2020

Repro is there:

https://github.com/crubier/snowpack-test

The same repo but using yarn v1 or yarn v2 with node_modules resolution works fine though. So definitely pnp related

@crubier
Copy link

crubier commented Oct 10, 2020

Ok for the record, the fix is to add this in snowpack.config.js:

module.exports = {
  mount: {
     ...
  },
  plugins: [
     ...
  ],
  installOptions: {
    rollup: {
      plugins: [require('rollup-plugin-pnp-resolve')()],
    },
  },
}

@FredKSchott
Copy link
Owner Author

Thanks for finding this! I'm surprised, according to this table Rollup is supposed to support yarn pnp by default: https://yarnpkg.com/features/pnp#pnp-loose-mode

@DreierF
Copy link
Contributor

DreierF commented Dec 4, 2020

The latest versions of snowpack (2.18.0, 3.0.0-rc.1)no longer seem to be compatible with yarn PnP.

To reproduce run:

$ npx create-snowpack-app snowpack-yarn--template @snowpack/app-template-react-typescript --use-yarn
$ cd snowpack-yarn
$ yarn set version berry
$ yarn config set pnpMode loose
$ yarn start
snowpack

▼ Console

[snowpack] p.match is not a function

(Adding rollup-plugin-pnp-resolve does not help as well)

I traced it down to yarnpkg/berry#899 which is open for quite some time already.
@FredKSchott Is there any chance that this could be fixed on the snowpack side?

@theseyi
Copy link

theseyi commented Dec 27, 2020

Which previous version of snowpack still compatible with berry @DreierF ? Thanks

EDIT: nvm, Using ~2.17.1 seems to work

@stephtr
Copy link
Contributor

stephtr commented Dec 27, 2020 via email

@theseyi
Copy link

theseyi commented Dec 27, 2020

snowpack@2.15.1 However you might have to add the following to your root package.json: "resolutions": { "esinstall": "0.3.6" }

Thanks @stephtr I'm actually using 2.17.1 and it seems to be ok so far? Should I downgrade to 2.15.1 or will I run into issues later?

@FredKSchott
Copy link
Owner Author

Shocked that yarn doesn't support this! We'll roll this change back for now until that Yarn issue is solved.

@stephtr
Copy link
Contributor

stephtr commented Dec 28, 2020

Thanks @stephtr I'm actually using 2.17.1 and it seems to be ok so far? Should I downgrade to 2.15.1 or will I run into issues later?

With 2.17.1 I ran into issues as soon as I started snowpack. If it's working for you, it should be fine, especially since downgrading is such a quick thing to do.

It seems like a fix for yarn will be close (yarnpkg/berry#1584).

@stephtr
Copy link
Contributor

stephtr commented Jan 2, 2021

Shocked that yarn doesn't support this! We'll roll this change back for now until that Yarn issue is solved.

In case you aren't subscribed to the Yarn issue, support just got added via yarnpkg/berry#2291. I successfully tested it by switching to the master branch version (using yarn set version from sources) 🎉

@thealjey
Copy link

thealjey commented Sep 16, 2022

@FredKSchott may I enquire what is the status of this issue?
All I'm doing is initializing a new yarn project with:

yarn init -2
yarn install
yarn add snowpack --dev

Then creating a snowpack.config.mjs.
However, running snowpack dev immediately after that, exits with an error -
Package "child_process" not found. Have you installed it?

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

No branches or pull requests

8 participants