Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

Support native ESM via .mjs. #433

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

jaydenseric
Copy link

@jaydenseric jaydenseric commented May 2, 2018

Node.js natively supports ESM via .mjs (currently behind the --experimental-modules flag, soon to be unflagged).

At the moment, consumers using native ESM, graphql and express-graphql are in a conundrum (see #425). When they use graphql the ESM .mjs modules run. When express-graphql internally uses graphql, the CJS .js files run. graphql thinks there are multiple versions being used and throws a tantrum. This issue will be mitigated by supporting native ESM across both packages.

  • Swapped deprecated babel-preset-es2015 for babel-preset-env.
  • Added package.json engines field.
  • Configured babel-preset-env to target the Node.js version defined in package.json engines field.
  • Kept Babel at v6, but moved config into a v7 ready .babelrc.js file for dynamic config via env.
  • Tidied the package.json scripts, added an ESM build step that generates .mjs dist files. Follows the lead of Use .mjs for module code graphql-js#1244, although I do things quite different for my own projects with Babel v7.
  • Updated package.json main field for ESM/CJS cross compatibility.
  • Added a package.json module field for tree-shaking bundlers.

I didn't upgrade Babel to v7 to limit the scope of this PR. It would be a good idea though.

Relates to graphql#425.

- Swapped deprecated babel-preset-es2015 for babel-preset-env.
- Added package.json engines field.
- Configured babel-preset-env to target the Node.js version defined in package.json engines field.
- Kept Babel at v6, but moved config into a v7 ready .babelrc.js file for dynamic config via env.
- Tidied the package.json scripts, added an ESM build step that generates .mjs dist files. Follow the lead of graphql/graphql-js#1244, although I do things quite different for my own projects with Babel v7.
- Updated package.json main field for ESM/CJS cross compatibility.
- Added a package.json module field for tree-shaking bundlers.
@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@jaydenseric
Copy link
Author

Don't merge yet, I'm noticing that dist/index.mjs has a few erroneous module.exports in it for some reason.

This one needs careful consideration.
@IvanGoncharov
Copy link
Member

Looks great 🎉

Don't merge yet, I'm noticing that dist/index.mjs has a few erroneous module.exports in it for some reason.

Please ping me when you solve this issue or if you need help with it.
I was planning on doing 0.7.0 at the end of this week and would be great to also include this feature.

Tidied the package.json scripts, added an ESM build step that generates .mjs dist files. Follows the lead of graphql/graphql-js#1244, although I do things quite different for my own projects with Babel v7.

If you have time you can help to migrate graphql-js: graphql/graphql-js#1266
It would be ideal to first update it and than all other graphql/* projects.

Added a package.json module field for tree-shaking bundlers.

Maybe it also make sense to add "sideEffects": false simular to graphql/graphql-js#1312

Added package.json engines field.

No need to support 4.0 since it's End-of-life so you can set it to 6.0
https://github.com/nodejs/Release

This is a nesesary breaking change. This is consistent with the graphql package, which only has named exports.
…nto mjs

# Conflicts:
#	src/__tests__/usage-test.js
@jaydenseric
Copy link
Author

As a breaking change, I updated the exports to be only named for ESM/CJS cross compatibility and consistency.

For a native ESM environment:

import { graphqlHTTP, getGraphQLParams } from 'express-graphql'

For a legacy CJS environment:

- const graphqlHTTP = require('express-graphql')
- const getGraphQLParams = graphqlHTTP.getGraphQLParams
+ const { graphqlHTTP, getGraphQLParams } = require('express-graphql')

The old way the additional getGraphQLParams export was added onto the graphqlHTTP function was pretty weird anyway, and now the import and require look consistent.

There is no concept of mixed default/named exports in CJS, only a babel convention of a .default export. If we had mixed, this is what the API would look like:

// ESM:
import graphqlHTTP, { getGraphQLParams } from 'express-graphql'

// CJS:
const { default: graphqlHTTP, getGraphQLParams } = require('express-graphql')

- Configured Travis to test on the latest stable Node.js version (v9-10+ were not being tested!), and the lowest supported version which is now v6. Removed unessesary versions in between.
- Updated package.json engines field, which in turn updates transpilation via dynamic Babel preset-env config.
@@ -2,10 +2,8 @@ language: node_js

# https://github.com/nodejs/LTS
node_js:
- "8"
Copy link
Member

Choose a reason for hiding this comment

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

8 is still in Active LTS and we should definitely test it + we need to test 10

Copy link
Author

Choose a reason for hiding this comment

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

v10 is tested as it is the current stable release used via 'node'.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to use explicit version, especially for consistency with graphql-js and other graphql/* repos.

{
modules: process.env.ESM ? false : 'commonjs',
targets: {
node: require('./package.json').engines.node.substring(2), // Strip `>=`
Copy link
Member

Choose a reason for hiding this comment

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

Until babel/babel#7277 is implemented I think it's better to use explicit string.

Copy link
Author

Choose a reason for hiding this comment

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

Why? Then you would not have a single source of truth.

Copy link
Member

@IvanGoncharov IvanGoncharov May 7, 2018

Choose a reason for hiding this comment

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

Because this construct assumes >= <version> and it's very brittle approach.
I personally think engines should contain only node version that we run tests against. See my PR for graphql-js: https://github.com/graphql/graphql-js/pull/1338/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R21

@IvanGoncharov
Copy link
Member

IvanGoncharov commented May 7, 2018

As a breaking change, I updated the exports to be only named for ESM/CJS cross compatibility and consistency.

@jaydenseric I would prefer not make such breaking changes especially since the absolute majority of users don't use getGraphQLParams function.
Can we make it backward compatible using babel-plugin-add-module-exports?

@jaydenseric
Copy link
Author

jaydenseric commented May 23, 2018

@IvanGoncharov

Can we make it backward compatible using babel-plugin-add-module-exports?

For starters, that plugin does not, and likely will not support babel v7.

The best way forward is to tweak the API to properly support both environments. As a best practice, no npm package should have mixed named/default exports. Only default, or only named. Only default is probably an inferior option if you expect your API to grow, as you can't export more stuff without a breaking change to named exports.

The new API would be consistent with graphql and graphql-relay, which only have named exports.

This issue is a dealbreaker for me and others I know. Because Apollo does not support native ESM either I've begun work on a new GraphQL server package.

@jdalton
Copy link

jdalton commented Jul 7, 2018

(currently behind the --experimental-modules flag, soon to be unflagged).

Hi 👋 ! Node core and Node Module WG member here. There is no set date for when --experimental-modules will be unflagged. There's also no guarantee that what has shipped now is what will be in the future. For example, there are several discussions in the Node Module WG on how to approach implementation. One such discussion is to ship a maximally minimal implementation that would require package-maps for resolving bare specifiers (no longer using the Node module resolution algorithm).

Adopting .mjs with the idea that its current support is solid (for production use) at this stage is bit premature and may cause headache for users. Not only ecosystem hurdles like #425 or how Webpack locks down bundled .mjs bits making your code more difficult to mock, stub, and spy. But also if/when changes do happen to Node's --experimental-modules your ESM story will be various shades of broken.

The --experimental-modules flag really does mean experimental. It's something you should pause and consider at least before going all in as the route for your ESM support strategy today.

@jaydenseric
Copy link
Author

Things were moving too slow so I published graphql-api-koa, and have been enjoying writing resolvers in native ESM for a month now without issue.

@FredrikZeiner
Copy link

We're about a month away from Node 12 going into LTS. What happened to this feature?

Base automatically changed from master to main February 10, 2021 15:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants