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

introduce pnpm, fix build tooling #3054

Closed
wants to merge 10 commits into from
Closed

introduce pnpm, fix build tooling #3054

wants to merge 10 commits into from

Conversation

acao
Copy link
Member

@acao acao commented Feb 25, 2023

  • migrate lockfile and workspaces config
  • github actions
  • tsc --build builds for all workspaces
  • vite build
  • webpack build
  • eslint errors
  • eslint warnings
  • jest resolution bugs (skipped fetch mock for now)
  • cypress?
  • changesets
  • typedoc

I almost forgot to look at #232, which has plenty of the missing pieces to borrow from, and give credit to the author as well! For example, they have taken care of github actions/etc already

@changeset-bot
Copy link

changeset-bot bot commented Feb 25, 2023

⚠️ No Changeset found

Latest commit: d48208e

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

@acao acao changed the title pnpm mode WIP pnpm WIP Feb 25, 2023
@acao acao force-pushed the pnpm branch 9 times, most recently from 5ac8635 to 4a2d1a6 Compare February 26, 2023 10:10
@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 branch 13 times, most recently from deb4850 to f4bcb59 Compare February 26, 2023 12:55
@@ -5,5 +5,5 @@ GraphQL implementation with parcel bundler.

### Setup

1. `yarn` and `yarn start` from this folder to start parcel dev mode.
1. `yarn build` to find production ready files.
1. `pnpm` and `pnpm start` from this folder to start parcel dev mode.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. `pnpm` and `pnpm start` from this folder to start parcel dev mode.
1. `pnpm i` and `pnpm start` from this folder to start parcel dev mode.

@@ -4,5 +4,5 @@ This example demonstrates how to transpile your own custom ES6 and typescript Gr

### Setup

1. `yarn` and `yarn start` from this folder to start `react-scripts` dev server.
1. `yarn build` from this folder to build production ready transpiled files using `react-scripts`. Find the output in `build` folder.
1. `pnpm` and `pnpm start` from this folder to start `react-scripts` dev server.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. `pnpm` and `pnpm start` from this folder to start `react-scripts` dev server.
1. `pnpm i` and `pnpm start` from this folder to start `react-scripts` dev server.

@@ -5,5 +5,5 @@ implementation with parcel bundler.

### Setup

1. `yarn` and `yarn start` from this folder to start parcel dev mode.
1. `yarn build` to find production ready files.
1. `pnpm` and `pnpm start` from this folder to start parcel dev mode.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. `pnpm` and `pnpm start` from this folder to start parcel dev mode.
1. `pnpm i` and `pnpm start` from this folder to start parcel dev mode.

@@ -10,4 +10,4 @@ This workspace could be used to help us prototype components & hooks for

## Setup

In this directory, you can just run `yarn` and `yarn start`
In this directory, you can just run `pnpm` and `pnpm start`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
In this directory, you can just run `pnpm` and `pnpm start`
In this directory, you can just run `pnpm i` and `pnpm start`

@@ -4,7 +4,7 @@ A simple example of `monaco-graphql` using webpack 4

### Setup

`yarn` and `yarn start` from this folder to start webpack dev server
`pnpm` and `pnpm start` from this folder to start webpack dev server
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
`pnpm` and `pnpm start` from this folder to start webpack dev server
`pnpm i` and `pnpm start` from this folder to start webpack dev server

@@ -35,10 +35,10 @@
"!jest.config.js"
],
"scripts": {
"build": "yarn build-clean && yarn build-js && yarn build-esm && yarn build-flow .",
"build": "yarn build-js && yarn build-esm && yarn build-flow .",
Copy link
Collaborator

@dimaMachina dimaMachina Apr 30, 2023

Choose a reason for hiding this comment

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

Should we use pnpm here and for other scripts in this file?

@@ -9,7 +9,7 @@ analyzer.html
/graphiql.*js.map
/graphiql.*css
/graphiql.*css.map
example/yarn.lock
example/pnpm.lock
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
example/pnpm.lock
example/pnpm-lock.yaml

Copy link
Member Author

Choose a reason for hiding this comment

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

can you tell I did a cheap bulk find replace and it's a mess ? haha

"e2e-server": "start-server-and-test 'cross-env PORT=8080 node test/e2e-server' 'http-get://localhost:8080/graphql?query={test { id }}'",
"storybook": "start-storybook",
"webpack": "webpack-cli --config resources/webpack.config.js"
},
"dependencies": {
"@graphiql/react": "^0.16.0",
"@graphiql/toolkit": "^0.8.1",
"@graphiql/toolkit": "^0.8.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"@graphiql/toolkit": "^0.8.0",
"@graphiql/toolkit": "workspace:*",

All monorepo packages/examples should contain the above

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm ok, and then they are rewritten on publish? i will have to double check how this works with changesets, because currently this is automatically managed by changesets

Copy link
Collaborator

Choose a reason for hiding this comment

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

and then they are rewritten on publish?

yes

@@ -1,3 +1,3 @@
node_modules
src
yarn.lock
pnpm.lock
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pnpm.lock
pnpm-lock.yaml


```sh
yarn global add graphql-language-service-cli
pnpm global add graphql-language-service-cli
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pnpm global add graphql-language-service-cli
pnpm add -g graphql-language-service-cli

@@ -35,6 +35,7 @@
"graphql": "^15.5.0 || ^16.0.0"
},
"dependencies": {
"@types/yargs": "^16.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be devdeps

@@ -1,3 +1,3 @@
node_modules
src
yarn.lock
pnpm.lock
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pnpm.lock
pnpm-lock.yaml

${bright('yarn global add graphql-language-service-cli')}
pnpm:
${bright('pnpm global remove graphql-language-service')}
${bright('pnpm global add graphql-language-service-cli')}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
${bright('pnpm global add graphql-language-service-cli')}
${bright('pnpm add -g graphql-language-service-cli')}

${bright('yarn global remove graphql-language-service')}
${bright('yarn global add graphql-language-service-cli')}
pnpm:
${bright('pnpm global remove graphql-language-service')}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
${bright('pnpm global remove graphql-language-service')}
${bright('pnpm remove -g graphql-language-service')}

@@ -301,7 +301,7 @@ This plugin uses the
[GraphQL language server](https://github.com/graphql/graphql-language-service-server)

1. Clone the repository - https://github.com/graphql/graphiql
1. `yarn`
1. `pnpm`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. `pnpm`
1. `pnpm i`

@@ -29,7 +29,7 @@ const git = async (...commands) => execa('git', commands, execOpts);
// async function preReleaseVSCode(version) {
// try {
// await execa(
// 'yarn',
// 'pnpm',
// ['workspace', `vscode-graphql`, 'run', 'release', '--pre'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// ['workspace', `vscode-graphql`, 'run', 'release', '--pre'],
// ['--filter', `vscode-graphql`, 'release', '--pre'],

@acao
Copy link
Member Author

acao commented May 2, 2023

@B2o5T if i recall, this is missing a bunch of my local commits that adds an entirely new build tool (tsup) and so much more, @B2o5T let's schedule a session to go over this later in the week? anyone else who wants to join is invited

@acao
Copy link
Member Author

acao commented May 3, 2023

@dotansimha and @B2o5T - i have pushed the afformentioned additional commit from changes that i had left on my local git stash from february, the very helpful review comments are still relevant. tonight after work I will clean it up and I'm available for a chat or async if you have any questions!

@acao acao marked this pull request as draft May 3, 2023 10:13
@acao
Copy link
Member Author

acao commented May 3, 2023

todo:

  • - cleanup errant find-replace changes
  • - rebase
  • - figure out tsconfig jsx recipe that prevents the need for react import
  • -

@dimaMachina
Copy link
Collaborator

figure out tsconfig jsx recipe that prevents the need for react import

@acao I think @thomasheyenbrock found some issues with it #3017

@acao
Copy link
Member Author

acao commented May 3, 2023

@B2o5T indeed, this PR was created after that, and required reverting the change he made there for now, but we will figure that out!

Comment on lines +29 to +30
vite.config.js
vite.config.d.ts
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you remove also generated files vite.config.js, vite.config.d.ts from packages/graphiql-react?

@acao
Copy link
Member Author

acao commented May 3, 2023

@B2o5T also, fwiw, feel free to push to this branch if you don't want to wait for me haha

@dimaMachina
Copy link
Collaborator

@B2o5T also, fwiw, feel free to push to this branch if you don't want to wait for me haha

@acao these files not present in your branch, but exists in main branch, you should do a rebase :)

@acao
Copy link
Member Author

acao commented May 3, 2023

@B2o5T indeed, i will try to get to it after work, maybe around 19:00 UTC. i just wanted to push what i had during lunch break real quick for reference

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.

3 participants