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

yarn => pnpm #2323

Closed
wants to merge 11 commits into from
Closed

yarn => pnpm #2323

wants to merge 11 commits into from

Conversation

rxliuli
Copy link

@rxliuli rxliuli commented Apr 19, 2022

ref: #2185

@changeset-bot
Copy link

changeset-bot bot commented Apr 19, 2022

⚠️ No Changeset found

Latest commit: b1e8074

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

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 19, 2022

CLA Missing ID CLA Not Signed

@rxliuli
Copy link
Author

rxliuli commented Apr 19, 2022

I have signed up for EasyCLA, what else do I need to do to trigger the next process? @acao

@acao
Copy link
Member

acao commented Apr 19, 2022

@rxliuli ah, the email used for CLA has to be the same email that’s used in each commit. Also I have to approve the action workflows for the first PR from a user because we have had malicious PRs

@acao
Copy link
Member

acao commented Apr 19, 2022

It looks like we need to use a base image that already has pnpm ideally, or we can use an action to manually install pnpm for each workflow

@rxliuli
Copy link
Author

rxliuli commented Apr 19, 2022

It looks like we need to use a base image that already has pnpm ideally, or we can use an action to manually install pnpm for each workflow

Just run npm i -g pnpm

Copy link
Member

@acao acao left a comment

Choose a reason for hiding this comment

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

Please update the github actions to install pnpm, preferrably in the base docker image, so that CI passes. As you can see, all 18 checks are failing

@acao acao marked this pull request as draft April 19, 2022 16:29
@rxliuli
Copy link
Author

rxliuli commented Apr 20, 2022

Do you want to change the command to install dependencies on netlify? I have observed that the dependencies are still installed using npm and then fail

https://app.netlify.com/sites/graphiql-test/deploys/625f4fd9e4fab7000994463a

@acao
Copy link
Member

acao commented Apr 20, 2022

@rxliuli awesome! Also, weird, not sure why i had to re-approve the actions

@rxliuli
Copy link
Author

rxliuli commented Apr 20, 2022

Build & Test I know what's wrong, it's fixed. But why does lint fail, it seems strange, do I need to fix these lint and ts errors? Or some configuration was accidentally changed. . .

https://github.com/graphql/graphiql/runs/6087316492?check_suite_focus=true

@acao
Copy link
Member

acao commented Apr 20, 2022

@rxliuli something is off with lint staged, so for now you need to run pnpm format.

@acao
Copy link
Member

acao commented Apr 20, 2022

I can’t remember why i migrated the actions to re-install modules from the shared cache individually rather than having a shared action for npm install and build. It started with a seperate install => cypress workflow that seemed to complete 1 minute faster somehow. Either way this is great because it will save us 30-50s per action based on your experiments

@acao
Copy link
Member

acao commented Apr 20, 2022

Hmm, the typescript version must be resolving differently, either way you can use the ts-expect-error comment if you want since they are mostly just handling exceptions

@rxliuli
Copy link
Author

rxliuli commented Apr 20, 2022

I restored the version of the dependencies from the previous yarn.lock, including typescript, sorry, I didn't notice the change of the dependency version before

@rxliuli rxliuli marked this pull request as ready for review April 20, 2022 01:55
@rxliuli
Copy link
Author

rxliuli commented Apr 21, 2022

webpack4 is not compatible with pnpm, it's a bit tricky, have you guys done webpack4 => webpack5? Or we can use rollup? or use faster esbuild?
webpack/webpack#5087

@timsuchanek
Copy link
Member

I think @acao was thinking of Vite, which I'd be totally ok with.
From my side you can go ahead and move to Vite.
Curious what @acao thinks

@acao
Copy link
Member

acao commented Apr 21, 2022

This was the blocker we had run into before with pnpm, thus why i raised the issue of webpack support early on! Our plan was to migrate to vite instead, and drop babel and webpack altogether. But we may have to upgrade to webpack 5 as a transitional step. It will have to wait as I’m sick with covid and my co maintainers are focused on other things at the moment.

when we originally upgraded to webpack from browserify (yes when I inherited the project it still used browserify), webpack 4 was beta and webpack 5 did not exist. So many generations of tooling 😭

@acao
Copy link
Member

acao commented Apr 21, 2022

With vite the goal was to get rid of the duplicate typescript project references tree and use vite to bundle each library for it‘s relevant exports, but I’m having trouble finding something that can re-create tsc —build —watch on a monorepo scale. @nx might have something for this, I can’t seem to find any vite and/or rollup plugins that solve this problem. Turnorepo purports to solve this problem but is only available commercially

@rxliuli
Copy link
Author

rxliuli commented Apr 21, 2022

This was the blocker we had run into before with pnpm, thus why i raised the issue of webpack support early on! Our plan was to migrate to vite instead, and drop babel and webpack altogether. But we may have to upgrade to webpack 5 as a transitional step. It will have to wait as I’m sick with covid and my co maintainers are focused on other things at the moment

Is there any suggestion, currently only one **Test PR / Cypress E2E Suite (pull_request) ** can not pass, I don't use webpack very much so don't know much about the configuration here
packages/graphiql/resources/webpack.config.js

@rxliuli
Copy link
Author

rxliuli commented Apr 21, 2022

With vite the goal was to get rid of the duplicate typescript project references tree and use vite to bundle each library for it‘s relevant exports, but I’m having trouble finding something that can re-create tsc —build —watch on a monorepo scale. @nx might have something for this, I can’t seem to find any vite and/or rollup plugins that solve this problem

In production, we wrote a plugin to handle this, pointing the module to src/index.ts so that we don't have to rebuild dist/index.js every time the lib is modified

ref: https://github.com/rxliuli/liuli-tools/blob/master/libs/rollup-plugin-ts-alias/README.md

@acao
Copy link
Member

acao commented Apr 21, 2022

Like I mentioned in #2185, you will need to find a compatibility mode or workaround for webpack 4, jest, cypress, etc, which has been what kept us from switching to pnpm im previous attempts. Currently the deploy preview and build users import from cdn isn’t building.

Otherwise a webpack 5 upgrade will take some sizeable effort and will definitely need to come in a separate PR, since it heavily impacts the actual bundle we ship to the CDNs, which is used more than even the esm module!

@acao
Copy link
Member

acao commented Apr 21, 2022

@rxliuli that plugin looks very promising thanks! Apologies there seems to be a delay with my LTE connection, each of your comments is only visible once I make a comment? So I’m just seeing your reply now

@acao
Copy link
Member

acao commented Apr 21, 2022

I think we can do this best in two or more PRs:

  1. swap out webpack for vite - we don’t need to worry about bundling every library yet. Leave duplicate project references for now. Merge.
  2. then, rebase this PR for pnpm with that upstream change
  3. Then, worry about replacing the duplicate references for a modern bundling and even exports strategy in an additional PR

@timsuchanek
Copy link
Member

Thanks for providing this context @acao ! That plan makes total sense to me.
@rxliuli do you have capacity to work on the steps Rikki outlined?

@rxliuli
Copy link
Author

rxliuli commented Apr 21, 2022

Thanks for providing this context @acao ! That plan makes total sense to me. @rxliuli do you have capacity to work on the steps Rikki outlined?

I will try it tomorrow

@rxliuli rxliuli closed this Jul 29, 2022
@acao acao reopened this Aug 14, 2022
@acao acao mentioned this pull request Feb 25, 2023
11 tasks
@acao
Copy link
Member

acao commented Mar 21, 2023

closing for #3054

@acao acao closed this Mar 21, 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.

4 participants