Build .mjs files (ES modules) alongside CommonJS #209
Conversation
This isn’t technically necessary for publishing ES modules, but I’ve gone ahead and aligned this with graphql-js anyway.
| @@ -7,6 +7,8 @@ | |||
| * @flow | |||
| */ | |||
|
|
|||
| import { Buffer } from './buffer'; | |||
|
|
|||
There was a problem hiding this comment.
As I understand facebook/flow#3723 was merged and will be available in the next release.
Does that mean you can just change this line to:
import Buffer from 'buffer';and everything will work?
There was a problem hiding this comment.
Ah, sadly no. We'd be able to simplify buffer.js#L28-L37 to
export const Buffer = NodeBuffer.Buffer;(which should now typecheck)
but we'd still have Node's actual implementation to deal with.
Under node --experimental-modules, import { Buffer } from 'buffer' fails because the buffer module doesn't export { Buffer } at all; it only has a default export with Buffer as a property. Hence, the rest of buffer.js is still needed for compatibility with both Node and Webpack, even once the Flow fix lands.
package.json
Outdated
| @@ -15,6 +15,8 @@ | |||
| "url": "http://github.com/graphql/graphql-relay-js.git" | |||
| }, | |||
| "main": "lib/index.js", | |||
There was a problem hiding this comment.
This should be "main": "lib", so the .mjs files can be resolved automatically in an ESM environment.
|
@motiz88 a similar effort graphql/express-graphql#433. You might like to adopt the Babel v7 ready approach to the dynamic config (.babelrc.js) here. With I didn't do it in my PR as I didn't want to rock the boat too much; I really need a speedy merge. Here are some published, working examples of the proposed approach: |
| * Flow (tested with 0.69.0) knows about global.Buffer but not about | ||
| * require('buffer').Buffer. | ||
| * https://github.com/facebook/flow/issues/3723 | ||
| */ |
There was a problem hiding this comment.
@motiz88 Flow 0.72 released and it includes your fix 🎉 Can you please update this PR?
|
@IvanGoncharov Done, thanks for pinging me. @jaydenseric re: |
| @@ -9,7 +9,7 @@ | |||
|
|
|||
| import { expect } from 'chai'; | |||
| import { describe, it } from 'mocha'; | |||
| import { StarWarsSchema } from './starWarsSchema.js'; | |||
| import { StarWarsSchema } from './starWarsSchema'; | |||
There was a problem hiding this comment.
@motiz88 If you have time maybe it worth to split such changes into separate PR.
I think it would be easy to find someone from Facebook willing to merge such simple change
+ it will make the main PR smaller
|
Guys, desperate for this. It seems there is nothing substantial blocking a merge? I'm about to copy paste functions into my repo, write my own, or publish this PR as a disposable package to npm or something. |
@jaydenseric I did exactly that you can find the package on npm the code on gitlab |
|
@motiz88 Sorry for the long delay. |
Closes #208, with the actual build changes lifted nearly verbatim from
graphql-js(e.g. graphql/graphql-js#1244)sideEffects: falsea la the recently-merged Mark all modules as having no side effects for bundling graphql-js#1312.Buffer) that would otherwise be encountered by users trying to actually bundle this as ESM with Webpack (e.g. what I'm currently doing withswapi-graphqlas a testbed).This is somewhat related to the issue with
processfixed in graphql/graphql-js@60c03ab - except here, getting Node, Webpack and Flow to agree took some non-obvious fine-tuning, so I broke the solution out into its own heavily-commented file (utils/buffer.js). The Flow part can be simplified onceAdd Buffer property to buffer module facebook/flow#6179hopefully lands.UPDATE: facebook/flow#6179 has landed and this branch incorporates the Flow 0.73 upgrade (which is also its own PR for good measure: #210)