CJS refactor.

[jaydenseric]

This PR refactors the project from ESM to CJS source and removes the Babel build step, resulting in no more ESM being published. In the process, several anonymous functions have been named, for better error stack traces.

The primary motive is to avoid CJS/ESM compatibility issues across recent Node.js versions, now that native ESM is likely to be unflaged any day now in Node.js v13.2.

I'm not sure this is a good idea yet.

Some nice things about publishing source CJS is that JSDoc comments are now included in the published code, and the published code is a lot tidier as it hasn't been spat out by Babel. The dev dependencies and scripts are a lot simpler.

Will it break some of the TypeScript types floating around the ecosystem?

An alternative is to keep ESM source and Babel, but build to a seperate ESM directory for a package module entrypoint, for bundlers like Webpack to utilize. This approach would make it easy to support native ESM again in the future:

• If a proper solution for dual CJS/ESM packages ends up shipping in Node.js, we can setup an appropriate build target.
• When we no longer have to support Node.js < v13.2 (likely the first version to support native ESM unflagged) we could just remove Babel, and publish the ESM source. This could be years away, as Node.js v12 LTS end of life is April 2022.

[mike-marcacci]

Will it break some of the TypeScript types floating around the ecosystem?

This shouldn't be a problem; the only caveat is that the the esModuleInterop setting must be true to allow named imports... but I suspect that option is already enabled in most projects anyhow.

And to be honest, that's not as much about types as it is about transpilation, since named imports of cjs files aren't natively possible in node either, so typescript is merely accessing the exported object.

[saostad]

I am not sure it's related or not but after I updated my node version to 13.1.0 my app broke with error Maximum call stack size exceeded
part of call stack:

internal/fs/streams.js in _openReadFs at line 120:21
internal/fs/streams.js in ReadStream.<anonymous> at line 116:3
internal/util.js in ReadStream.deprecated [as open] at line 70:15
/home/node/app/node_modules/fs-capacitor/lib/index.js in ReadStream.open at line 90:11
internal/fs/streams.js in _openReadFs at line 123:12
internal/fs/streams.js in ReadStream.<anonymous> at line 116:3
internal/util.js in ReadStream.deprecated [as open] at line 70:15
/home/node/app/node_modules/fs-capacitor/lib/index.js in ReadStream.open at line 90:11
internal/fs/streams.js in _openReadFs at line 123:12
internal/fs/streams.js in ReadStream.<anonymous> at line 116:3
internal/util.js in ReadStream.deprecated [as open] at line 70:15
/home/node/app/node_modules/fs-capacitor/lib/index.js in ReadStream.open at line 90:11
internal/fs/streams.js in _openReadFs at line 123:12
internal/fs/streams.js in ReadStream.<anonymous> at line 116:3
internal/util.js in ReadStream.deprecated [as open] at line 70:15
/home/node/app/node_modules/fs-capacitor/lib/index.js in ReadStream.open at line 90:11
internal/fs/streams.js in _openReadFs at line 123:12
internal/fs/streams.js in ReadStream.<anonymous> at line 116:3
internal/util.js in ReadStream.deprecated [as open] at line 70:15

I downgraded to node 12.x to fix the problem

[jaydenseric]

@saostad Node.js v13 support will come when the next version is published:

• https://github.com/jaydenseric/graphql-upload/blob/master/changelog.md#next
• Upgrade to fs-capacitor v4 #166

[mike-marcacci]

Hey @jaydenseric! Just to double check, is this waiting on me? I haven't really jumped in yet, since it's marked as draft, but I'm happy to review it if that would be helpful.

[jaydenseric]

@mike-marcacci this PR is ready for review, but having slept on it I'm leaning to not going down this path.

What do you think about publishing what is on master branch now (with a little additional docs polish), and not changing the ESM build strategy until Node.js ships conditional exports? They are hinting it will land no later than January 2020. Also, by then the tooling story for native ESM should have improved for dev dependencies such as Babel. It will probably be good timing as we'll probably ship a major version then anyway, dropping end-of-life Node.js v8.

In the meantime, Node.js v13 will be supported but it can only pick up the CJS files, so named imports won't work. To remove confusion, the code examples should be updated to be CJS instead of ESM.

[mike-marcacci]

This looks very thorough – interesting choice to export most of these functions as root module.exports = exports instead of adding them as keys. I'd imagine this may make future interop easier?

The whole saga of modules and node is unfortunate. I know it isn't entirely self-inflicted (I've heard that the node team felt their use case wasn't adequately considered by TC39 when designing the spec) but the lack of a timely interoperability story is just prolonging the period where the "lowest common denominator" is the only solution for shipping libraries.

Until a strategy appears that can solve the problem more broadly, I just don't know if the value of supporting es modules is worth the effort, especially with incompatible experimental support across different versions.

I think this PR is probably a solid approach for now. However, if you have the energy to keep up, I certainly applaud the initiative! I hadn't heard about conditional exports until now – that's a very interesting idea.