[tooling] retire CJS for libraries #3324

acao · 2023-07-05T16:03:54Z

It's 2023 and we don't need commonjs anymore.
graphql-js will drop cjs for 16

This PR:

removes all main entrypoints
removes any unneeded commonjs related config and tools for libraries
leaves the build directory as esm for now, for legacy import support? what do we think of this choice? or should we standardize on dist? I only change this for codemirror-graphql, but this keeps monaco-graphql/esm imports intact. if only imports key in package.json accepted * 😢
simplifies the project references build
if you're using graphql-lsp, upgrade
transparently builds all packages with a build script in order of dependency

CJS still exists for:

vscode projects because vscode requires commonjs targer -@graphiql/react and graphiql, where legacy consumers may need them
TODO i think we will need it for cm6-graphql still for tests

After this:

turbopack
drop build-bundles?
targeted dev scripts

changeset-bot · 2023-07-05T16:03:58Z

🦋 Changeset detected

Latest commit: 60d2a40

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages

Name	Type
codemirror-graphql	Major
graphql-language-service	Major
graphql-language-service-cli	Major
graphql-language-service-server	Major
monaco-graphql	Major
cm6-graphql	Minor
@graphiql/react	Minor
@graphiql/toolkit	Minor
graphiql	Patch
vscode-graphql	Patch
@graphiql/plugin-explorer	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-07-05T16:06:22Z

The latest changes of this PR are available as canary in npm (based on the declared changesets):

[email protected]
[email protected]
[email protected]
@graphiql/[email protected]
@graphiql/[email protected]
@graphiql/[email protected]
[email protected]
[email protected]
[email protected]
[email protected]

It's 2023 and we don't need commonjs anymore. `graphql-js` will drop cjs for 16 This PR: - removes all `main` entrypoints - removes any commonjs related config and tools for libraries - leaves the build directory as `esm` for now, for legacy import support - simplifies the project references build - if you're using `graphql-lsp`, upgrade CJS still exists for: - vscode projects because vscode requires commonjs targer -`@graphiql/react` and `graphiql`, where legacy consumers may need them - TODO i think we will need it for cm6-graphql still for tests

dimaMachina · 2023-07-06T08:27:33Z

or should we standardize on dist?

I prefer dist, also you can add type: module in package.json to avoid mjs extensions

acao · 2023-07-06T08:53:04Z

@B2o5T it seems to work fine without mjs extensions

why introduce another breaking change when we've already documented monaco-graphql/esm everywhere?

acao · 2023-07-06T09:06:44Z

@B2o5T I agree with your point that dist makes more sense... and while we are making breaking changes and bumping versions... we might as well document this in the changelog and examples

we will need to make a PR to the vite monaco plugin to update their docs

dimaMachina · 2023-07-06T09:14:12Z

@acao it’s per this recommendation https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c#how-can-i-move-my-commonjs-project-to-esm

and about monaco-graphql - it's already a major release so we could introduce breaking changes, also I would keep only the following exports:

monaco-graphql with initialize mode, not ugly monaco-graphql/esm/initializeMode
some exports for workers

so it simplifies things without monaco-graphql/{dist,esm}/something

Also, I would setup this rule https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/filename-case.md for consistency kebab-case filenames that we use in supported The Guild projects

and please hold this PR to merge until I approve it

acao · 2023-07-06T09:16:18Z

already one step ahead 😆

acao · 2023-07-06T09:22:28Z

I will get vitest working again, and make a few more tweaks, this is almost ready for a final review!

acao · 2023-07-06T09:40:11Z

adding "type": "module" will take some more time, and will require converting a bunch of scripts and configs from require to import, I will have to come to that later. this will not be merged without much more review don't worry, haha

acao · 2023-07-06T09:41:27Z

We will also need to figure out why this effort regressed before this can have the final review. it doesn't seem to be picking up the typings directory now when being imported

which is why this vitest is failing

I'm worried this test will be flaky as monaco-editor changes, is there maybe a better way to test? maybe ensure it has less than n modules?

dimaMachina · 2023-07-06T09:42:26Z

and will require converting a bunch of scripts and configs from require to import

you can temporarily renames their extension to cjs to avoid such as convertion

acao · 2023-07-06T09:45:56Z

@B2o5T indeed, but the jest side of things will be tricky, I will need to spend a few more days on this to finish the esm conversion

dimaMachina · 2023-07-06T09:46:01Z

Also tests fail with the following error

AssertionError: expected '✓ 969 modules transformed.' to include '✓ 1092 modules transformed.

nice 👍 we have fewer converted modules 😂

acao · 2023-07-06T09:48:44Z

@B2o5T haha nice! thus why i suggest using the "less than" approach suggested in the comment above. it seems github comments are slow for us today 😆

I think this may be caused by a regression where monaco-graphql/dist/typings/monaco-editor.d.ts is not being picked up. the branch in it's current state fails the example builds

Speaking of, I'm considering whether we should make the example monaco build and one of the graphiql examples part of the netlify build again. This allowed us to validate consuming types previously, and then we had a /monaco or /webpack path to demonstrate the example build output on netlify. it's slower but worth it. this was what build-demo used to do

dimaMachina · 2023-07-06T09:48:48Z

maybe ensure it has less than n modules?

I guess it is good to keep the exact number of dependencies, I think we'll adjust this number very rarely when vite version changes or such as this migration

dimaMachina · 2023-07-06T09:51:28Z

I think this may be caused by a regression where

I can take a look later this week, currently on vacation

acao · 2023-07-06T09:52:58Z

@B2o5T no worries, I will figure it out! enjoy your holiday

acao · 2023-07-06T09:55:23Z

I guess it is good to keep the exact number of dependencies, I think we'll adjust this number very rarely when vite version changes or such as this migration

This is what I'm worried about. This can be caused by either changes to vite or monaco-editor, and as a rule of thumb, I prefer to not write tests that test internal details of third party dependencies. I still think less than n will be more ideal, otherwise this might frustrate contributors as just small patch resolution changes could break this

acao · 2023-07-06T10:09:31Z

@B2o5T ah i found the issue, had overlooked your script that patches types in renaming from esm to dist paths

dimaMachina · 2023-07-06T18:51:16Z

packages/graphiql-react/package.json

      "types": "./types/index.d.ts"
    },
    "./font/roboto.css": "./font/roboto.css",
    "./font/fira-code.css": "./font/fira-code.css",
-    "./dist/style.css": "./dist/style.css"


I guess it worked well before?

it did, but this didnt work with webpack-cli serve

acao · 2023-07-07T12:10:39Z

@B2o5T i think this is good to merge now, and then your type": "module" effort can be a follow up PR, just to make the effort more manageable and scoped

packages/graphiql-plugin-code-exporter/package.json

dimaMachina · 2023-07-07T12:25:57Z

packages/graphiql-plugin-explorer/package.json

@@ -6,7 +6,6 @@
    "url": "https://github.com/graphql/graphiql",
    "directory": "packages/graphiql-plugin-explorer"
  },
-  "main": "dist/index.js",


This is needed, we still export cjs

examples/monaco-graphql-react-vite/vite.config.ts

packages/graphiql-react/package.json

packages/graphiql/package.json

dimaMachina · 2023-07-07T12:41:03Z

@B2o5T i think this is good to merge now, and then your type": "module" effort can be a follow up PR, just to make the effort more manageable and scoped

Please wait for merge such as big PR

you can always point your future work on this target branch
I am not sure about graphiql react projects if you still bundle cjs or not, or what you decided
I wanna polish exports for other packages to be true esm
You can’t merge asap such as big changes in 2 days, I didn't review/tested a lot of things, no reasons for rush

acao · 2023-07-07T12:49:47Z

yes we can wait and fine tune it, and I would not release until all of these PRs are complete, but what I'm pointing out is that #3327 can wait until this is merged, because of we merge #3327 into this branch it will be very complicated

When everything is ready, it can go like this:

complete & merge this PR that retires cjs for libraries, and simplifies the build tooling
complete & merge make monaco-graphql true esm with "type": "module" and "exports" field, simplify it's exports #3327 and other efforts to migrate to full "type": "module" as an additional effort, instead of merging into this branch
merge several other graphiql related efforts
release

Overall, I think a more gradual approach to these iterations before next release is best

acao · 2023-07-08T16:07:36Z

just to clarify @B2o5T, the purpose of this PR is not to move the whole monorepo to type: module. it is just to remove CJS exports for libraries, but not for consuming packages.

the effort of moving to "type": "module" is much greater and not required for this, as we can see with vite and webpack (including next js) happily consuming the esm only exports

the effort to move the repo to use esm only will require migration away from jest which does not support type: module, and changes to many files and tooling. these tooling changes are not required to only publish esm, despite how nonsensical it seems to use cjs to publish esm only libraries, this is sufficient for our users. the rest of the tooling migration will happen, but it's not the greatest priority right now.

I'm trying to keep these PRs small, towards simplifying the repo tooling so we can make other improvements. As I mention in the last comment, the effort to move monaco-graphql to "type": "modules" can happen seperately, as it is not required anymore by bundlers, unlike when sindre published that gist

sorry this wasn't clear when this PR began, and now that I've seen the effort involved with more experiments today, I just want to draw a clear line of the scope here!

acao · 2023-08-17T03:15:19Z

I've learned from shipping other libraries to webpack consumers that commonjs exports are still required, so this is probably a bad idea. type: module is one thing, but dropping main and commonjs exports is another.

acao added the tooling label Jul 5, 2023

acao changed the title ~~retire CJS for libraries~~ [tooling] retire CJS for libraries Jul 5, 2023

acao force-pushed the retire-cjs branch 6 times, most recently from d1fce1c to c85ec08 Compare July 5, 2023 19:43

acao force-pushed the retire-cjs branch from c85ec08 to c5fafc3 Compare July 6, 2023 08:17

finally get rid of manual build scripts

82c50ab

remove unused script

b5ce792

move esm/ to dist/

3b582e7

dist for lsp server & cm6

e22a51b

acao force-pushed the retire-cjs branch from c7cf269 to e22a51b Compare July 6, 2023 09:18

cleanup files declarations

b632c37

add type: module

780f166

acao force-pushed the retire-cjs branch from 1d73540 to 780f166 Compare July 6, 2023 09:32

get rid of type: module for now to get tests passing

81500fb

acao added 2 commits July 6, 2023 14:36

fix vitest

d0b9144

babel tweak, messy setup for graphiql-dev

470f9ed

dimaMachina reviewed Jul 6, 2023

View reviewed changes

acao added 2 commits July 7, 2023 13:13

try a serial build

bf74e23

fixed!

6abe07c

dimaMachina reviewed Jul 7, 2023

View reviewed changes

packages/graphiql-plugin-code-exporter/package.json Show resolved Hide resolved

dimaMachina reviewed Jul 7, 2023

View reviewed changes

examples/monaco-graphql-react-vite/vite.config.ts Show resolved Hide resolved

dimaMachina reviewed Jul 7, 2023

View reviewed changes

packages/graphiql-react/package.json Outdated Show resolved Hide resolved

dimaMachina reviewed Jul 7, 2023

View reviewed changes

packages/graphiql/package.json Show resolved Hide resolved

acao added 2 commits July 7, 2023 15:16

normalize vite builds

4729702

fix webpack build

1ee1efc

fix bin path

60d2a40

acao mentioned this pull request Jul 13, 2023

simplify yarn build, fixes #3335 #3345

Merged

acao closed this Aug 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tooling] retire CJS for libraries #3324

[tooling] retire CJS for libraries #3324

acao commented Jul 5, 2023 •

edited

Loading

changeset-bot bot commented Jul 5, 2023 •

edited

Loading

github-actions bot commented Jul 5, 2023 •

edited

Loading

dimaMachina commented Jul 6, 2023 •

edited

Loading

acao commented Jul 6, 2023 •

edited

Loading

acao commented Jul 6, 2023 •

edited

Loading

dimaMachina commented Jul 6, 2023

acao commented Jul 6, 2023

acao commented Jul 6, 2023

acao commented Jul 6, 2023

acao commented Jul 6, 2023 •

edited

Loading

dimaMachina commented Jul 6, 2023 •

edited

Loading

acao commented Jul 6, 2023

dimaMachina commented Jul 6, 2023

acao commented Jul 6, 2023 •

edited

Loading

dimaMachina commented Jul 6, 2023

dimaMachina commented Jul 6, 2023

acao commented Jul 6, 2023

acao commented Jul 6, 2023

acao commented Jul 6, 2023

dimaMachina Jul 6, 2023

acao Jul 7, 2023

acao commented Jul 7, 2023 •

edited

Loading

dimaMachina Jul 7, 2023

dimaMachina commented Jul 7, 2023

acao commented Jul 7, 2023 •

edited

Loading

acao commented Jul 8, 2023 •

edited

Loading

acao commented Aug 17, 2023

[tooling] retire CJS for libraries #3324

[tooling] retire CJS for libraries #3324

Conversation

acao commented Jul 5, 2023 • edited Loading

changeset-bot bot commented Jul 5, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Jul 5, 2023 • edited Loading

dimaMachina commented Jul 6, 2023 • edited Loading

acao commented Jul 6, 2023 • edited Loading

acao commented Jul 6, 2023 • edited Loading

dimaMachina commented Jul 6, 2023

acao commented Jul 6, 2023

acao commented Jul 6, 2023

acao commented Jul 6, 2023

acao commented Jul 6, 2023 • edited Loading

dimaMachina commented Jul 6, 2023 • edited Loading

acao commented Jul 6, 2023

dimaMachina commented Jul 6, 2023

acao commented Jul 6, 2023 • edited Loading

dimaMachina commented Jul 6, 2023

dimaMachina commented Jul 6, 2023

acao commented Jul 6, 2023

acao commented Jul 6, 2023

acao commented Jul 6, 2023

dimaMachina Jul 6, 2023

Choose a reason for hiding this comment

acao Jul 7, 2023

Choose a reason for hiding this comment

acao commented Jul 7, 2023 • edited Loading

dimaMachina Jul 7, 2023

Choose a reason for hiding this comment

dimaMachina commented Jul 7, 2023

acao commented Jul 7, 2023 • edited Loading

acao commented Jul 8, 2023 • edited Loading

acao commented Aug 17, 2023

acao commented Jul 5, 2023 •

edited

Loading

changeset-bot bot commented Jul 5, 2023 •

edited

Loading

github-actions bot commented Jul 5, 2023 •

edited

Loading

dimaMachina commented Jul 6, 2023 •

edited

Loading

acao commented Jul 6, 2023 •

edited

Loading

acao commented Jul 6, 2023 •

edited

Loading

acao commented Jul 6, 2023 •

edited

Loading

dimaMachina commented Jul 6, 2023 •

edited

Loading

acao commented Jul 6, 2023 •

edited

Loading

acao commented Jul 7, 2023 •

edited

Loading

acao commented Jul 7, 2023 •

edited

Loading

acao commented Jul 8, 2023 •

edited

Loading