Skip to content

Use NPM as default package manager#80

Closed
jonkoops wants to merge 1 commit intothgh:mainfrom
jonkoops:use-npm
Closed

Use NPM as default package manager#80
jonkoops wants to merge 1 commit intothgh:mainfrom
jonkoops:use-npm

Conversation

@jonkoops
Copy link
Copy Markdown
Collaborator

@jonkoops jonkoops commented Aug 7, 2023

Switches the package manager from Yarn to NPM. This replaces the old Yarn lockfile with an NPM version. There are a couple of reasons for doing this:

  1. I'll be introducing some automation using GitHub Actions, this simplifies the setup needed for it.
  2. It makes things more familiar for external contributors (for example, see Cross-origin isolation support in Chrome  #67, where multiple lock files are checked in).
  3. All features we would need that Yarn supports are also in NPM now, making it less useful.

@jonkoops jonkoops requested a review from thgh August 7, 2023 15:27
@jonkoops
Copy link
Copy Markdown
Collaborator Author

jonkoops commented Aug 7, 2023

@thgh if you feel comfortable with giving me merge rights, I can move this along without a review. I'll be putting out quite a couple of PRs to keep things reviewable, so it could be favorable to do so.

If you prefer to review them individually before merging, that is also fine by me. Let me know what works for you.

@jonkoops
Copy link
Copy Markdown
Collaborator Author

jonkoops commented Aug 7, 2023

Side-note, this PR should be merged before any PRs that modify the package file (such as #82 and #83), as these will influence the lockfile as well.

@thgh
Copy link
Copy Markdown
Owner

thgh commented Aug 7, 2023

I would recommend against this. I don't agree with your reasons. If you can automate testing by switching to NPM, that's a good reason though!

As a collaborator, I think you have merge rights. The main branch requires a review though.

Ideally, you create 1 PR with all changes required to fix the types issue. If a PR doesn't fix something, I'm hesitant to merge. Don't fix what's not broken.

@jonkoops
Copy link
Copy Markdown
Collaborator Author

My main reason for this is that when setting up automation I will not have to set up Yarn. For example, actions/setup-node assumes NPM and installing Yarn is another step that is not required.

Ideally, you create 1 PR with all changes required to fix the types issue.

I'll do this separately. I like to make breaking changes explicit commits, so the changelog can be easily compiled from the commit history.

@thgh
Copy link
Copy Markdown
Owner

thgh commented Aug 14, 2023

setup-node installs yarn by default, will close this as you can do this in the @rollup/plugins repo

@thgh thgh closed this Aug 14, 2023
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.

2 participants