Skip to content

chore: upgrade to [email protected] #2678

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 41 commits into from
Closed

chore: upgrade to [email protected] #2678

wants to merge 41 commits into from

Conversation

zakuru
Copy link
Contributor

@zakuru zakuru commented Apr 10, 2023 •
edited
Loading

Summary

Chore:

  1. Upgrade to [email protected]
  2. Setup corepack to use pnpm version 8.2.0
  3. Update yaml files node matrix to target node 16 and exclude node 14
  4. Update yaml files to use pnpm version 8.2.0 via corepack command

Related issue (if exists)

Types of changes

  • [ X] Docs change / Dependency upgrade
  • Bug fix
  • New feature / Improvement
  • Refactoring
  • Breaking change

Checklist

  • I have added changeset via pnpm run changeset.
  • I have added tests to cover my changes.

@changeset-bot
Copy link

changeset-bot bot commented Apr 10, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: 8590c2b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@zakuru zakuru changed the title Chore: Upgrade to [email protected] Upgrade to [email protected] Apr 10, 2023
@zakuru zakuru changed the title Upgrade to [email protected] chore: upgrade to [email protected] Apr 10, 2023
@zakuru
Update package.json
52212d9 
node >=16.14
@zakuru zakuru marked this pull request as draft April 10, 2023 03:08
hyf0
hyf0 reviewed Apr 10, 2023
View reviewed changes
@zakuru zakuru marked this pull request as ready for review April 10, 2023 06:56
@hyf0 hyf0 mentioned this pull request Apr 11, 2023
Closed
@hyf0 hyf0 added the on hold label Apr 11, 2023
@hyf0 hyf0 assigned hyf0 and unassigned hardfist Apr 11, 2023
@hyf0 hyf0 modified the milestones: Planned, Working in progress Apr 11, 2023
@hyf0 hyf0 mentioned this pull request Apr 14, 2023
Merged
@hyf0 hyf0 removed the on hold label Apr 15, 2023
hyf0
hyf0 reviewed Apr 15, 2023
View reviewed changes
.github/workflows/reusable-build.yml Outdated
@@ -188,7 +188,7 @@ jobs:
strategy:
fail-fast: false
matrix:
node: [14, 16]
node: [16]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove 14?

Copy link
Contributor Author

@zakuru zakuru Apr 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why remove 14?

About this part its for 2 reasons:

It is recommend using node 16 in the contribution doc https://github.com/web-infra-dev/rspack/blob/main/CONTRIBUTING.md#install-nodejs so I though it would make sense to align the version in the CI as well
pnpm version 8 requires node version 16 ++

Copy link
Contributor

Choose a reason for hiding this comment

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

pnpm version 8 requires node version 16 ++

I miss this. Rspack is guaranteed to be running in node14 and node16. If that's the case, I'm not sure we should update to pnpm@8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure no worries. I was just following the docs, let me know when you have updates
Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pnpm version 8 requires node version 16 ++

I miss this. Rspack is guaranteed to be running in node14 and node16. If that's the case, I'm not sure we should update to pnpm@8.

Seems that the change brings some complications
I can close it with no concern if its the case

Copy link
Contributor

Choose a reason for hiding this comment

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

We would have a meeting tomorrow to discuss this PR and related topics. You could keep the PR open until the team come up with some conclusions.

Copy link
Contributor

@hyf0 hyf0 Apr 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sync up:

We have a quick discussion just now. We would use a fixed version, which is the latest version of pnpm@7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hyf0 sure here it is #2780

hyf0
hyf0 reviewed Apr 15, 2023
View reviewed changes
@hyf0 hyf0 added the to be discussed Rspack team would discuss these issues per week label Apr 16, 2023
@zakuru zakuru marked this pull request as draft April 17, 2023 04:51
@zakuru zakuru marked this pull request as ready for review April 17, 2023 05:21
@zakuru zakuru marked this pull request as draft April 17, 2023 05:51
@zakuru zakuru marked this pull request as ready for review April 17, 2023 07:39
@zakuru zakuru closed this Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hyf0 hyf0 hyf0 left review comments

@IWANABETHATGUY IWANABETHATGUY Awaiting requested review from IWANABETHATGUY

@h-a-n-a h-a-n-a Awaiting requested review from h-a-n-a

@hardfist hardfist Awaiting requested review from hardfist

Assignees

@hyf0 hyf0

Labels
to be discussed Rspack team would discuss these issues per week
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zakuru @hardfist @hyf0