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 note about Yarn 2 support. #21657

Merged
merged 1 commit into from
Jan 29, 2021
Merged

Add note about Yarn 2 support. #21657

merged 1 commit into from
Jan 29, 2021

Conversation

leerob
Copy link
Member

@leerob leerob commented Jan 29, 2021

As we saw some confusion with users trying out Yarn 2 and hitting roadblocks in their app. Yarn 2 is supported on a best-effort basis.

@kodiakhq kodiakhq bot merged commit 843252b into vercel:canary Jan 29, 2021
@SleeplessByte
Copy link

(Thanks!)

@@ -41,6 +41,8 @@ npm install next react react-dom
yarn add next react react-dom
```

> **Note**: [Yarn 2](https://yarnpkg.com/getting-started/install) is supported as best-effort within Next.js. For best results, use NPM or Yarn 1.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yarn 2 !== Yarn PnP, just like with Yarn 1 you can use both PnP and node_modules so this should really be updated as this isn't accurate. The migration guide explicitly states to start with node_modules then enable PnP once ready

Choose a reason for hiding this comment

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

The issue that sprung this PR was multiple place on GitHub it's "Yarn 2 is supported". By default Yarn 2 will enable PnP. So yes, I agree 100% with your statement, but that's not what in practice will happen when people try to use Yarn 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

but that's not what in practice will happen when people try to use Yarn 2.

If people skip the migration guide I'd argue that is their problem and not something that should cause Next.js to advise against Yarn 2

Choose a reason for hiding this comment

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

In essence I agree with you, but that's not reality. Yarn 2 defaults to PnP. There is no call to action to migrate. Next.js just states "it works with Yarn 2", not "You must follow the migration guide".

It's very easy to say that it's other people's problem, but it's equally elitist to do so.

@Timer
Copy link
Member

Timer commented Jan 29, 2021

I think we should remove this comment, @merceyz has been adding extensive e2e tests for Yarn 2 support and the only issue here was a result of using npm + Yarn for disabling telemetry.

I want us to have full Yarn 2 compat, so this message doesn't really make sense.

@SleeplessByte
Copy link

(I am happy to help with any of these wherever I can. I just have my "help our juniors out"-hat on right now. Feel free to shout out).

and the only issue here was a result of using npm + Yarn for disabling telemetry.

No, Yarn PnP doesn't work with browserlist's update either! So even when Next.js is fully compatible, there are still upstream dependencies that are not.

@Timer
Copy link
Member

Timer commented Jan 29, 2021

Can you open an issue with browserslist for PnP compat? Really, you should only need to run yarn upgrade or npm upgrade... I don't know why they have a proprietary command.

kodiakhq bot pushed a commit that referenced this pull request Jan 29, 2021
My addition of the Yarn 2 caveat was more nuanced than I thought. Apologies. @merceyz has been adding extensive e2e tests for Yarn 2 support. We plan to support Yarn 2 compat.

Reverts #21657.
@SleeplessByte
Copy link

@Timer I previously tracked that down and (luckily) already exists: browserslist/browserslist#577

@vercel vercel locked as resolved and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants