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

pnpm + turbo (no cjs + no ts project refs) #3306

Draft
wants to merge 49 commits into
base: main
Choose a base branch
from
Draft

Conversation

acao
Copy link
Member

@acao acao commented Jun 29, 2023

this one is for real, not just for reference!

  • drop cjs
  • drop typescript project references
  • add turbo
  • replace yarn with pnpm
  • add build and dev packages to every workspace
  • prettier
  • drop build-bundles
  • vscode compile -> build
  • proper exports for codemirror-graphql
  • eslint
  • cypress failures - something with the minor bundling tweaks is causing codemirror-graphql jsonParser exceptions to not be caught as linter errors??
  • webpack dev server - build works but dev server 404s
  • changsets upgrade
  • typedoc

Notes

  • we may need to leave project references in place just so that we can resolve projects for the latest typedoc. I don't think there is a way around this for typescript. we don't have to use typescript project references for the build of course
  • running pnpm dev alongside cypress open or jest, a common workflow for me when working across packages, seems to crash my PC
  • for some reason, the @graphiql/react tsconfig.json was configured to compile __test__ directory. this was not needed for jest-ts but seemed to make eslint happy, but i found a way to resolve this

What can hopefully come in later PRs:

  • migrate graphiql webpack bundler to vite
  • monaco language bundling fix
  • replace tsc usage with bob, tsup, etc
  • typescript upgrade
  • replace jest with vitest
  • import react properly
  • etc

it's a more lean version of #3303 with some agreed upon tooling changes

@changeset-bot
Copy link

changeset-bot bot commented Jun 29, 2023

⚠️ No Changeset found

Latest commit: e5e3a4b

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.

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

💥 An error occurred when fetching the changed packages and changesets in this PR
Some errors occurred when validating the changesets config:
The package or glob expression "example-*" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

@acao acao force-pushed the pnpm-turbo-no-cjs branch 3 times, most recently from 4fd5433 to 7d8967a Compare June 29, 2023 19:34
@acao acao force-pushed the pnpm-turbo-no-cjs branch from 7d8967a to b83005d Compare June 29, 2023 19:36
@acao acao force-pushed the pnpm-turbo-no-cjs branch 2 times, most recently from 982e740 to d6f7a71 Compare June 29, 2023 20:07
@acao acao force-pushed the pnpm-turbo-no-cjs branch from d6f7a71 to 34809cc Compare June 29, 2023 20:08
@github-actions
Copy link
Contributor

The latest changes of this PR are not available as canary, since there are no linked changesets for this PR.

@acao acao force-pushed the pnpm-turbo-no-cjs branch from 4bd70da to e288d27 Compare June 29, 2023 22:02
@acao acao changed the title pnpm turbo no cjs pnpm + turbo + no cjs + no ts project refs Jun 29, 2023
@acao acao force-pushed the pnpm-turbo-no-cjs branch from 407a532 to 39afb66 Compare June 29, 2023 22:50
@acao acao force-pushed the pnpm-turbo-no-cjs branch from 77ed1a7 to efa69e7 Compare June 30, 2023 10:38
@acao
Copy link
Member Author

acao commented Jun 30, 2023

@B2o5T please note that when a PR is in draft state, it is not ready to review. I appreciate the early review, but things are subject to change drastically.

This is everything that is needed to move to pnpm. In order to move to pnpm with project references, we would have to go deeper into using project references which you said you didn't want to do. Thus why pnpm and turbo are coming together. Moving to pnpm requires upgrading changesets, typedoc, others. Removing cjs simplifies the repo and the PR - otherwise the dev tasks would be very expensive, each incurring two parallel tsc builds

Other preliminary work has already happened - webpack upgrade, etc. All of these changes are a consequence of migrating to pnpm.

For changes that can happen in smaller PRs, I plan to seperate them out, thus why this PR is a draft. Please, to save you time, don't review until it is is out of draft state, ok?

@acao acao force-pushed the pnpm-turbo-no-cjs branch 2 times, most recently from 1d9eea6 to ce46877 Compare July 1, 2023 10:31
@acao acao force-pushed the pnpm-turbo-no-cjs branch from ce46877 to 920483a Compare July 1, 2023 10:32
@acao acao force-pushed the pnpm-turbo-no-cjs branch from 3a57e13 to 0c4af1f Compare July 1, 2023 10:54
@acao
Copy link
Member Author

acao commented Jul 1, 2023

yay! cypress now runs using something closer to the reccomended config. we had to move cypress back into root devDependencies as per the cypress reccomendation

@acao acao force-pushed the pnpm-turbo-no-cjs branch from 8fbd8ab to 06619a2 Compare July 1, 2023 11:05
@acao
Copy link
Member Author

acao commented Jul 2, 2023

@B2o5T I will include instructions soon on how to try this out and start giving very specifically scoped feedback and/or contribution if you are interested. It has somehow introduced a few detectable regressions, see notes. The vscode extension tests will take precedence as they are of a greater concern

Copy link

@Noodleontour Noodleontour left a comment

Choose a reason for hiding this comment

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

Cool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants