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

make monaco-graphql true esm with "type": "module" and "exports" field, simplify it's exports #3327

Draft
wants to merge 14 commits into
base: retire-cjs
Choose a base branch
from

Conversation

dimaMachina
Copy link
Collaborator

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Jul 7, 2023

⚠️ No Changeset found

Latest commit: a41d699

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 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

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

"scripts": {
"build": "cross-env NODE_ENV=production webpack-cli",
"start": "cross-env NODE_ENV=development webpack-cli serve"
"bundle": "cross-env NODE_ENV=production webpack-cli",
Copy link
Collaborator Author

@dimaMachina dimaMachina Jul 7, 2023

Choose a reason for hiding this comment

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

renamed to bundle because build from root fails since I added this example to workspace and postbuild are executed too late for this example (should be fixed after setup of turbo)

@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023

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

self.onmessage = () => {
globalThis.onmessage = () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

monaco-editor uses globalThis in latest version instead of self

@@ -21,17 +44,14 @@
"directory": "packages/monaco-graphql"
},
"files": [
"dist",
"src",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know I already wrote it but let's stop publish src dir

Comment on lines -48 to +68
"monaco-editor": ">= 0.20.0 < 1",
"monaco-editor": ">= 0.39.0 < 1",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

update as per Rikki suggestion for next major

Copy link
Member

Choose a reason for hiding this comment

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

is there a reason we need 0.39.0 specifically? is there a more forgiving range we can support? we should document what we use from the 0.39.0 release that deprecates support for 0.38.0 etc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No reason, which version you propose to bump?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@acao I was rethinking this, since in monaco-editor every minor version is major, according to semver policy (since major is 0) let's bump it as highest as possible (0.40.0 in this case)

@dimaMachina
Copy link
Collaborator Author

dimaMachina commented Jul 7, 2023

@acao something was broken with your latest updates, build succeeded and all examples worked well until I merged with latest changes of your PR

@dimaMachina dimaMachina marked this pull request as draft July 7, 2023 08:42
@acao
Copy link
Member

acao commented Jul 7, 2023

@B2o5T yes, there is a race condition where the graphiql build begins before the @graphiql/react build is complete. previously, this worked because the tsc --noEmit was fast enough, but by adding the dts plugin to vite and dropping that, it has to wait for the full vite build which errors. turbo would fix this because it would only execute the next build in the dependency tree once the graphiql react build is complete

@acao
Copy link
Member

acao commented Jul 7, 2023

@B2o5T try rebasing again, it should work better now

@@ -9,7 +9,16 @@
'@graphiql/toolkit': minor
---

_BREAKING CHANGE:_ drop commonjs exports in all libraries except for `graphiql` and `@graphiql/react`
_BREAKING CHANGE:_ drop commonjs exports in all libraries except for `graphiql`
and `@graphiql/react`
Copy link
Member

Choose a reason for hiding this comment

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

why is this on a new line? it's harder to read

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some locale prettier changes, will revert 👍

@acao acao changed the title make monaco-graphql true esm with "type": "module" and "exports" field, simplify his exports make monaco-graphql true esm with "type": "module" and "exports" field, simplify it's exports Jul 7, 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.

2 participants