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

deploy full ESM with "type": "module" in "latest-esm" npm tag #3361

Merged
merged 1 commit into from
Sep 21, 2022

Conversation

PabloSzx
Copy link

@PabloSzx PabloSzx commented Nov 4, 2021

I also just re-published [email protected] as [email protected] https://npm.im/graphql-esm, tested it in an example project using libraries with full ESM support https://github.com/PabloSzx/test-graphql-esm, everything works perfectly 👌

It can be tested right now doing this:

{
  "dependencies": {
    "graphql": "npm:graphql-esm@^16.0.1"
  }
}

@PabloSzx PabloSzx marked this pull request as ready for review November 4, 2021 03:34
@PabloSzx PabloSzx changed the title deploy full ESM with "type": "module" deploy full ESM with "type": "module" in "esm" npm tag Nov 4, 2021
Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

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

Please add integrationTests/esm for this package.
Try to test as many esm-specific things as possible.

@IvanGoncharov
Copy link
Member

Also please ensure that this package works with upcoming TS 4.5 that should support esm natively and add integration tests for that.

package.json Outdated Show resolved Hide resolved
@PabloSzx PabloSzx marked this pull request as draft November 7, 2021 15:29
@PabloSzx PabloSzx force-pushed the deploy-esm branch 2 times, most recently from 1f2d271 to 48cfc07 Compare November 10, 2021 02:11
@PabloSzx PabloSzx marked this pull request as ready for review November 10, 2021 02:12
.gitignore Show resolved Hide resolved
resources/gen-version.js Outdated Show resolved Hide resolved
Copy link
Contributor

@yaacovCR yaacovCR left a comment

Choose a reason for hiding this comment

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

Looks good to me!

looks like esm tag makes sense out of band from canaries and all that

@yaacovCR
Copy link
Contributor

Looks good to me!

looks like esm tag makes sense out of band from canaries and all that

@IvanGoncharov is this ready for re-review?

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Sep 2, 2022
Motivation: allow to reuse more stuff from 'resources/utils.ts'
Working on rebasing graphql#3361 that adds even more code into integrationTests
so want to keep it simple by reusing code from 'utils.ts'
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Sep 2, 2022
Motivation: allow to reuse more stuff from 'resources/utils.ts'
Working on rebasing graphql#3361 that adds even more code into integrationTests
so want to keep it simple by reusing code from 'utils.ts'
@IvanGoncharov IvanGoncharov changed the title deploy full ESM with "type": "module" in "esm" npm tag deploy full ESM with "type": "module" in "latest-esm" npm tag Sep 20, 2022
@netlify
Copy link

netlify bot commented Sep 20, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 18f15ce
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/632a177293eb0200091946a7
😎 Deploy Preview https://deploy-preview-3361--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.

@IvanGoncharov

This comment has been minimized.

@github-actions
Copy link

@github-actions publish-pr-on-npm

@IvanGoncharov The latest changes of this PR are available on NPM as
graphql@17.0.0-alpha.1.canary.pr.3361.04ab27334641e170ce0e05bc927b972991953882
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-3361

@IvanGoncharov
Copy link
Member

I rebased this PR to the latest main as the solution for #3603

@IvanGoncharov IvanGoncharov merged commit 87d003d into graphql:main Sep 21, 2022
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Sep 21, 2022
@IvanGoncharov IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label Sep 23, 2022
@yaacovCR
Copy link
Contributor

@IvanGoncharov @PabloSzx et al

question about the current state => the build scripts add a build qualifier of "+esm" and a tag suffix of "-esm", but do not rename the package to graphql-esm => the integration tests that use graphql-esm just point to a local tarball with the distribution, presumably where the name of the package is still graphql.

Over at the graphql-esm on npm -- it doesn't seem like we have used the exact machinery within this repo to publish as on npm graphql-esm is up to 16.5 and the tag is just 'latest' not 'latest-esm'.

So, where are we with this? Some options:

(1) we can get rid of the build qualifiers and tag suffix, and have a package name suffix of "-esm" and every time we publish to graphql, we should also publish to graphql-esm....
(2) we can get rid of the cjs version and also get rid of the build qualifiers and tag suffix.

So either way, the end goal will be to get rid of the build qualifier and tag suffix, right?

But have we decided on what will happen?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants