graphiql icon indicating copy to clipboard operation
graphiql copied to clipboard

yarn => pnpm

Open rxliuli opened this issue 2 years ago • 24 comments

ref: https://github.com/graphql/graphiql/issues/2185

rxliuli avatar Apr 19 '22 09:04 rxliuli

⚠️ No Changeset found

Latest commit: b1e80749e70586de9f78d82d467a71b29afe24d8

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

changeset-bot[bot] avatar Apr 19 '22 09:04 changeset-bot[bot]

CLA Missing ID CLA Not Signed

  • :white_check_mark: login: rxliuli / name: rxliuli (39bc87ef30c2c984acc826789c6a6693060e8dee, d6a0b90c8ac129f58945120b0f5d14f0e6d2929c, c3e9ec0044f305d22548a62762416ef6dfbfb018, b1e80749e70586de9f78d82d467a71b29afe24d8)
  • :x: The commit (7d78754f9b6fe3b1b91bf061c6d18a66fc1783c2, 912c516e086e8de7964a0272476f88cec1b34ed3, ae6291387a59f452b4b6098192fa8588d4c3fa35, 04113a2f747c13b6fe3ded76b13bbd486b43bc54, 51fc85c4e48685fafe21969283baf88ae714c531, 1e3f102fcd81254a699ed9accde2bab5acb810ac, 186a5a1ac4bcc25ac131fd814daf825ef3b3543d). This user is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

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

rxliuli avatar Apr 19 '22 10:04 rxliuli

@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 avatar Apr 19 '22 10:04 acao

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

acao avatar Apr 19 '22 11:04 acao

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

rxliuli avatar Apr 19 '22 11:04 rxliuli

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

rxliuli avatar Apr 20 '22 00:04 rxliuli

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

acao avatar Apr 20 '22 00:04 acao

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

rxliuli avatar Apr 20 '22 00:04 rxliuli

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

acao avatar Apr 20 '22 00:04 acao

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 avatar Apr 20 '22 01:04 acao

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

acao avatar Apr 20 '22 01:04 acao

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 avatar Apr 20 '22 01:04 rxliuli

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? https://github.com/webpack/webpack/issues/5087

rxliuli avatar Apr 21 '22 06:04 rxliuli

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

timsuchanek avatar Apr 21 '22 07:04 timsuchanek

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 avatar Apr 21 '22 07:04 acao

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

acao avatar Apr 21 '22 07:04 acao

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 avatar Apr 21 '22 07:04 rxliuli

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

rxliuli avatar Apr 21 '22 07:04 rxliuli

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 avatar Apr 21 '22 07:04 acao

@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 avatar Apr 21 '22 08:04 acao

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

acao avatar Apr 21 '22 08:04 acao

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?

timsuchanek avatar Apr 21 '22 08:04 timsuchanek

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 avatar Apr 21 '22 12:04 rxliuli

closing for #3054

acao avatar Mar 21 '23 18:03 acao