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

Migrate from yarn to pnpm #2417

Closed
wants to merge 14 commits into from
Closed

Migrate from yarn to pnpm #2417

wants to merge 14 commits into from

Conversation

rudyflores
Copy link
Contributor

@rudyflores rudyflores commented Aug 10, 2023

Proposed changes

Migrate to new package manager PNPM for improved install and build times for development vs Yarn

Release Notes

Milestone: v3.0

Changelog: Changed Yarn package manager to PNPM package manager.

Testing this PR

For testing this PR perform the following:

  • run pnpm i --shamefully-hoist=true (this parameter is used to maintain hositing can be set as default in npmrc) https://pnpm.io/npmrc#shamefully-hoist
  • run pnpm build
  • run pnpm package
  • run pnpm test
  • run pnpm fresh-clone (this one uses the new concurrency feature!)
  • try running any npm or yarn command (this should throw an error since only pnpm commands are allowed)

Make sure to test normal ZE functionality in the dev host as well as packaged extension to make sure everything is in order as well :)

Metrics

fresh-clone command:

Yarn
Screen Shot 2023-08-10 at 3 08 36 PM

PNPM
Screen Shot 2023-08-10 at 3 08 56 PM

install command:

Yarn
Screen Shot 2023-08-10 at 3 11 10 PM

PNPM
Screen Shot 2023-08-10 at 3 10 17 PM

package command:

Yarn
Screen Shot 2023-08-10 at 3 13 23 PM

PNPM
Screen Shot 2023-08-10 at 3 14 44 PM

Types of changes

What types of changes does your code introduce to Zowe Explorer?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Updates to Documentation or Tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the reviewer

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • PR title follows Conventional Commits Guidelines
  • PR Description is included
  • gif or screenshot is included if visual changes are made
  • yarn workspace vscode-extension-for-zowe vscode:prepublish has been executed
  • All checks have passed (DCO, Jenkins and Code Coverage)
  • I have added unit test and it is passing
  • I have added integration test and it is passing
  • There is coverage for the code that I have added
  • I have tested it manually and there are no regressions found
  • I have added necessary documentation (if appropriate)
  • Any PR dependencies have been merged and published (if appropriate)

Further comments

Signed-off-by: Rudy Flores <[email protected]>
@rudyflores rudyflores self-assigned this Aug 10, 2023
Signed-off-by: Rudy Flores <[email protected]>
Signed-off-by: Rudy Flores <[email protected]>
@rudyflores
Copy link
Contributor Author

Seems like there is an audit issue with two dependencies needing an update, should we update this here or has it been already addressed and we are waiting for it to be merged into next? @JillieBeanSim @zFernand0

@rudyflores rudyflores marked this pull request as ready for review August 10, 2023 17:16
Signed-off-by: Rudy Flores <[email protected]>
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (3a89fc9) 91.89% compared to head (afb70f4) 91.89%.
Report is 9 commits behind head on next.

❗ Current head afb70f4 differs from pull request most recent head 950da5c. Consider uploading reports for the commit 950da5c to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2417   +/-   ##
=======================================
  Coverage   91.89%   91.89%           
=======================================
  Files          94       94           
  Lines        8734     8734           
  Branches     1766     1766           
=======================================
  Hits         8026     8026           
  Misses        707      707           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rudyflores
Copy link
Contributor Author

FYI Seems like a Theia check is failing due to integration tests failing, it's also related to a v1 profile test which seems to be missing it's function, I believe this is because we removed all v1 functions and perhaps the next branch hasn't gotten the changes from main yet.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

@zFernand0
Copy link
Member

zFernand0 commented Aug 15, 2023

I might be doing something wrong, or maybe I should've installed pnpm not via npm, but... 😋

  • is anyone else getting a bunch of  ERR_PNPM_ERR_PNPM_TARBALL_INTEGRITY ?
    And since I can't install, I'm not able to run the build, and other commands in the description 😢

How to reproduce?

  • echo "@zowe:registry=https://zowe.jfrog.io/zowe/api/npm/npm-local-release/" >> ~/.npmrc
  • pnpm store prune
  • pnpm install --shamefully-hoist=true

@rudyflores
Copy link
Contributor Author

  • ERR_PNPM_ERR_PNPM_TARBALL_INTEGRITY

I would probably do a fresh clone before installing since there may be some conflicting files from yarn

Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

Thanks @rudyflores for performing this migration 🙂

So far pnpm has been working great for me when testing this branch. A few questions:

  • I can't review the PR easily because there a lot of changes that seem unrelated to pnpm, like new files added in .github/actions and several changes made to unit tests. Perhaps the PR needs to be rebased against a different commit?
  • If the shamefully-hoist option is required, can we add it to an .npmrc file in the repo so that we don't have to remember to add it on the end of all our install commands? We could probably also remove the .yarnrc file and migrate it to .npmrc 😋

@zFernand0
Copy link
Member

  • ERR_PNPM_ERR_PNPM_TARBALL_INTEGRITY

I would probably do a fresh clone before installing since there may be some conflicting files from yarn

Thanks Rudy, but even after a fresh-clone, I'm still getting the same errors 🤔

@rudyflores
Copy link
Contributor Author

Thanks @rudyflores for performing this migration 🙂

So far pnpm has been working great for me when testing this branch. A few questions:

  • I can't review the PR easily because there a lot of changes that seem unrelated to pnpm, like new files added in .github/actions and several changes made to unit tests. Perhaps the PR needs to be rebased against a different commit?
  • If the shamefully-hoist option is required, can we add it to an .npmrc file in the repo so that we don't have to remember to add it on the end of all our install commands? We could probably also remove the .yarnrc file and migrate it to .npmrc 😋

Good suggestions, I think I will have to cherry pick some commits from this PR, and do this from a new branch, seems like the conflicts I attempted to resolve coming from next caused some commits unrelated to this PR be attached, which is why all those irrelevant files are popping up.

As for the shamefully-hoist I agree, this could be a good fix for this and that way the regular pnpm install command can be run alone without issue.

@zFernand0
Copy link
Member

zFernand0 commented Aug 16, 2023

The installation worked fine for me, but only after updating/creating an .npmrc file with the following contents

registry=https://registry.npmjs.org/
@zowe:registry=https://registry.npmjs.org/

Should we update the pnpm-lock.yaml so taht the resolutions are against the
@zowe:registry=https://zowe.jfrog.io/zowe/api/npm/npm-local-release/ ?


Also, there is a leftover v8-compile-cache-0 directory at the root of the project after running pnpm install --shamefully-hoist=true. should it be ignored, or is there a flag that can help to clean it up?

@rudyflores
Copy link
Contributor Author

that might be good as well, I can go ahead and update the lock file.

I might have to research the docs to see if that can be cleaned, otherwise we could always ignore the file too!

@rudyflores
Copy link
Contributor Author

Closing this PR in favor of #2424

@rudyflores rudyflores closed this Aug 18, 2023
@rudyflores rudyflores deleted the migrate-to-new-tools branch August 18, 2023 19:48
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.

4 participants