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

Use yarn instead of npm in docs and scripts #796

Merged
merged 4 commits into from Jul 4, 2022
Merged

Conversation

AndrewFerr
Copy link
Member

This is on top of #794, which should be merged first.

CONTRIBUTING.md Outdated
@@ -23,6 +23,8 @@ inside the `MatrixEventProcessor` class.

* You will need to [setup the bridge](https://github.com/Half-Shot/matrix-appservice-discord/tree/develop#setup-the-bridge) similarly to how we describe,
but you should setup a homeserver locally on your development machine. We would recommend [Synapse](https://github.com/matrix-org/synapse).
* We recommend using `yarn` for dependency management (v1 / Classic for the time being).
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL that there are newer versions. Are there any known incompatibilities that could bite us/users?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yarn Berry installs in a completely different incompatible layout to Yarn Classic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, probably be a good idea to enforce node and yarn versions in package.json so users don't get bitten.

Signed-off-by: Andrew Ferrazzutti <andrewf@element.io>
Signed-off-by: Andrew Ferrazzutti <andrewf@element.io>
@AndrewFerr
Copy link
Member Author

Enforcing Yarn was a lot more involved than I expected!

This uses the recommendation from yarnpkg/yarn#4895 (comment) to tweak package.json to forbid usage of npm and set a requirement on which yarn version is used.

Since that doesn't actually install the required Yarn version, I added a package script to call handle that. It calls yarn set version (which appears to be supported by Yarn 1.x despite being documented only in later versions) to download & lock the latest 1.x release.

Note that the recommended solution from the Yarn team is to lock versions with Corepack, or to keep the Yarn binary in version control. Since Corepack is currently opt-in and checking in a script is cumbersome, I settled on using the package.json tweak.

Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Looks okay, I'm very worried about locking out r package manager versions and forgetting it. Would rather we just failed to run on yarn 2+ entirely.

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Instead, just document how to install it

Signed-off-by: Andrew Ferrazzutti <andrewf@element.io>
Signed-off-by: Andrew Ferrazzutti <andrewf@element.io>
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

I am good with this change

@AndrewFerr AndrewFerr merged commit e25a474 into develop Jul 4, 2022
@AndrewFerr AndrewFerr deleted the af/npm-to-yarn branch September 1, 2022 17:38
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.

None yet

4 participants