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

feat: use pnpm instead npm #852

Merged
merged 9 commits into from
Apr 16, 2023
Merged

feat: use pnpm instead npm #852

merged 9 commits into from
Apr 16, 2023

Conversation

await-ovo
Copy link
Contributor

@await-ovo await-ovo commented Apr 7, 2023

Summary

The main advantages of using pnpm are:

  • save local disk space

  • improve installation speed

  • avoid the problem that the first npm run build of a newly cloned repository has a high probability of error(It often occurs in our ci process):

    ✖  @svgr/plugin-jsx:build
       > @svgr/[email protected] build
       > rollup -c ../../build/rollup.config.mjs


       src/index.ts → ./dist/index.js...
       created ./dist/index.js in 84ms

       src/index.ts → ./dist/index.d.ts...
       src/index.ts(7,37): error TS2307: Cannot find module '@svgr/core' or its corresponding type declarations.
       src/index.ts(38,28): error TS7006: Parameter 'code' implicitly has an 'any' type.
       src/index.ts(38,34): error TS7006: Parameter 'config' implicitly has an 'any' type.
       src/index.ts(38,42): error TS7006: Parameter 'state' implicitly has an 'any' type.

       [!] (plugin dts) RollupError: Failed to compile. Check the logs above.
  • avoid phantom dependency

Test plan

  • After pnpm install, the build and test both succeed.
  • vercel dev run success and in api directory after pnpm install.
  • publish packages using lerna, refer to the documentation here

I need help to verify that the release is working properly

Because netlify always fails for some reason when deployed (local is fine), Keep the website still using npm.

@vercel
Copy link

vercel bot commented Apr 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svgr ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 7, 2023 1:01pm

@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Merging #852 (d14751b) into main (649fa65) will not change coverage.
The diff coverage is n/a.

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

@@           Coverage Diff           @@
##             main     #852   +/-   ##
=======================================
  Coverage   92.80%   92.80%           
=======================================
  Files          32       32           
  Lines         751      751           
  Branches      249      249           
=======================================
  Hits          697      697           
  Misses         53       53           
  Partials        1        1           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gregberge
Copy link
Owner

@await-ovo very good thank you! Could you try to run lerna? You will be blocked on npm but everything else should work right?

@await-ovo
Copy link
Contributor Author

await-ovo commented Apr 7, 2023

@await-ovo very good thank you! Could you try to run lerna? You will be blocked on npm but everything else should work right?

Because I'm not familiar with lerna, do I need to run pnpm lerna version first and then pnpm run release, I've been doing this locally and it gives me an error every time:

$ pnpm release --dry-run

> @ release /Users/Documents/projects/svgr
> lerna publish --conventional-commits && conventional-github-releaser --preset angular "--dry-run"

lerna notice cli v6.6.0
lerna info current version 7.0.2
lerna notice Current HEAD is already released, skipping change detection.
lerna success No changed packages to publish 
/Users/Documents/projects/svgr/node_modules/.pnpm/[email protected]/node_modules/minimist-options/index.js:101
                                        throw new TypeError(`Expected "${key}" default value to be of type "${expectedType}", got ${prettyPrint(defaultType)}`);
                                        ^

TypeError: Expected "token" default value to be of type "string", got "undefined"

@await-ovo
Copy link
Contributor Author

I think we should verify that the workspace protocol is replaced with the specific version when the package is released.

@gregberge
Copy link
Owner

@await-ovo I think I will merge and try to make a release. Thanks!

@gregberge gregberge merged commit 79233c0 into gregberge:main Apr 16, 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