Skip to content

Conversation

IvanGoncharov
Copy link
Member

@IvanGoncharov IvanGoncharov added the PR: breaking change 💥 implementation requires increase of "major" version number label May 1, 2022
@netlify
Copy link

netlify bot commented May 1, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 946ab96
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/626ee36e8d928c0009e04997
😎 Deploy Preview https://deploy-preview-3552--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented May 1, 2022

Hi @IvanGoncharov, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@saihaj
Copy link
Member

saihaj commented May 1, 2022

@IvanGoncharov what about #3361 ?

@IvanGoncharov
Copy link
Member Author

@IvanGoncharov what about #3361 ?

@saihaj As we agreed, let's try with full ESM first, and if the community strongly asks for CJS we can go the #3361 approach with dual packages.

@saihaj
Copy link
Member

saihaj commented May 1, 2022

@IvanGoncharov what about #3361 ?

@saihaj As we agreed, let's try with full ESM first, and if the community strongly asks for CJS we can go the #3361 approach with dual packages.

I mean the work was already done there and we just need to that esm only package under main graphql npm

@IvanGoncharov
Copy link
Member Author

I mean the work was already done there and we just need to that esm only package under main graphql npm

@saihaj Can't do that, benchmark and integration tests need to use ESM, it's the absolute majority of changes in this PR.
#3361 don't do that.

@IvanGoncharov IvanGoncharov merged commit db6525e into graphql:main May 2, 2022
@IvanGoncharov IvanGoncharov deleted the pr_branch branch May 2, 2022 09:03
@saihaj
Copy link
Member

saihaj commented May 2, 2022

I mean the work was already done there and we just need to that esm only package under main graphql npm

@saihaj Can't do that, benchmark and integration tests need to use ESM, it's the absolute majority of changes in this PR. #3361 don't do that.

@IvanGoncharov we have tests in that PR
image

@saihaj
Copy link
Member

saihaj commented May 2, 2022

ok but we can just push a commit on initial work done there and wait for @graphql/graphql-js-reviewers team for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: breaking change 💥 implementation requires increase of "major" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants