n1ru4l/graphql-live-query

Improve package publishing

Opened this issue · 10 comments

dburles commented

Suggestions:

  1. The exports field shouldn't stripped from the published package.json and no index.mjs file is included. @n1ru4l I found that you raised this issue earlier, (though not sure why it was closed) kamilkisiela/bob#16
  2. Don't bundle. Publish as commonjs unbundled with both commonjs and ESM entrypoints. This can also allow consumers to deep import files rather than via their named exports. This should be encouraged in the documentation. The entrypoints can remain mostly for static analysis.
  3. As an alternative, perhaps consider perhaps only publishing ESM. https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c
n1ru4l commented
  1. There is an exports field in the package.json https://unpkg.com/browse/@n1ru4l/json-patch-plus@0.1.2/package.json Can you clarify what you mean?
  2. I dont like that as it does not allow renaming and moving internal stuff without making it a breaking change. What are the actual benefits of doing that?
  3. Only shipping ESM disrupts the ecosystem. Supporting both is not hard. I ao ne point in dropping commonjs support right now.
dburles commented
  1. There is an exports field in the package.json https://unpkg.com/browse/@n1ru4l/json-patch-plus@0.1.2/package.json Can you clarify what you mean?
  2. I dont like that as it does not allow renaming and moving internal stuff without making it a breaking change. What are the actual benefits of doing that?
  3. Only shipping ESM disrupts the ecosystem. Supporting both is not hard. I ao ne point in dropping commonjs support right now.

  1. I realised that I was on earlier versions of a couple of packages, which didn't include the exports field (https://unpkg.com/browse/@n1ru4l/graphql-live-query-patch-jsondiffpatch@0.4.0/package.json). I wasn't aware that it had already been addressed. Great!

  2. On deep imports, the advantage is that users won't be importing anything they don't need, saves memory on the server and negates the need to tree shake if bundling on the client. It also better supports browser URL imports over CDN since users won't be downloading the entire API (Edit: note that this requires publishing files as ESM, though some CDN's can convert to ESM such as https://esm.sh/).

  3. Supporting both is kinda hard, currently these packages all suffer from the dual package hazard, see here for an explanation.

  • 👍2
n1ru4l commented

If you want to reduce memory usage you can still bundle your server code and drop unused code via tree shaking. I don't see why deep imports (which limit me as a library maintainer), should take care of this instead.

I think importing from CDN and causing request waterfalls is also something that should be avoided for production usage (by using a bundler), for development/demos the additional loaded code won't be a dealbreaker.

The dual package hazard is only introduced if people decide to only ship ESM or commonjs which basically means "I don't care about library consumers" to me.

If a package has zero dependencies (or all dependencies of that package are ESM only) I agree that only shipping ESM is totally legit. But as soon your library has dependencies you should ship both commonjs and ESM in order to avoid the dual package hazard.

  • 👍3
  • 👎3
dburles commented

If you want to reduce memory usage you can still bundle your server code and drop unused code via tree shaking. I don't see why deep imports (which limit me as a library maintainer), should take care of this instead.

There would be no need for the complexities of bundling and tree shaking if authors would publish API's in this way. Users won't be able to deep import just anything, only what you choose to export as the public API.

The dual package hazard is only introduced if people decide to only ship ESM or commonjs which basically means "I don't care about library consumers" to me.

If a package has zero dependencies (or all dependencies of that package are ESM only) I agree that only shipping ESM is totally legit. But as soon your library has dependencies you should ship both commonjs and ESM in order to avoid the dual package hazard.

This is incorrect. The issue is that these packages are publishing both ESM and commonjs in seperate bundles, that will cause a dual package hazard. See here for an explanation: https://nodejs.org/api/packages.html#packages_dual_package_hazard. Essentially the same singletons should be exported from both ESM and commonjs entrypoints.

If you continue bundling, just have the ESM entrypoint import and export from the bundled commonjs entrypoint. Ideally, though as there's no real reason to bundle, just publish unbundled commonjs and have the two entrypoints import and export from those files.

n1ru4l commented

@dburles Can you provide a reproduction, where this library would be affected by dual package hazard in a real-world scenario? I would love to get some more insights on this, as it seems that I cannot fully understand the situation.

dburles commented

Hey @n1ru4l, this repo gives a pretty good example where the problem can arise https://github.com/GeoffreyBooth/dual-package-hazard

n1ru4l commented

@dburles Can you provide an example woth packages in this mono-repo?

dburles commented

@dburles Can you provide an example woth packages in this mono-repo?

That is not always easy to determine ahead of time, which is why it's a hazard that's best avoided, since it could manifest itself in unexpected ways. Given that this project is published in a very modular way, I would guess that you might expect others to potentially publish their own graphql-live-query utility package that consumes parts of the graphql-live-query API, and because of that, users are then prone to potential issues.

Anything publishing dual packages with https://github.com/kamilkisiela/bob suffers from the dual package hazard, since it creates two copies of the same code as both cjs and esm. If you wish to continue publishing in this way (which I wouldn't recommend), it would be best to maintain an awareness of the pitfalls, as explained here: https://nodejs.org/api/packages.html#approach-2-isolate-state.

It's a complex thing to grasp, so I hope that I've helped explain it a little. The Node docs do explain it very well, so have a read through that if you haven't already, https://nodejs.org/api/packages.html#dual-commonjses-module-packages.

dburles commented

For some additional clarity, these packages are currently published correctly if they included only the module field. However, since they've been updated to export ESM with conditional exports they are now prone to this issue.

I'll quote part of the Node docs on the dual package hazard as I think it will also be useful to others reading this issue:

Prior to the introduction of support for ES modules in Node.js, it was a common pattern for package authors to include both CommonJS and ES module JavaScript sources in their package, with package.json "main" specifying the CommonJS entry point and package.json "module" specifying the ES module entry point. This enabled Node.js to run the CommonJS entry point while build tools such as bundlers used the ES module entry point, since Node.js ignored (and still ignores) the top-level "module" field.

Node.js can now run ES module entry points, and a package can contain both CommonJS and ES module entry points (either via separate specifiers such as 'pkg' and 'pkg/es-module', or both at the same specifier via Conditional exports). Unlike in the scenario where "module" is only used by bundlers, or ES module files are transpiled into CommonJS on the fly before evaluation by Node.js, the files referenced by the ES module entry point are evaluated as ES modules.

Dual package hazard

When an application is using a package that provides both CommonJS and ES module sources, there is a risk of certain bugs if both versions of the package get loaded. This potential comes from the fact that the pkgInstance created by const pkgInstance = require('pkg') is not the same as the pkgInstance created by import pkgInstance from 'pkg' (or an alternative main path like 'pkg/module'). This is the “dual package hazard,” where two versions of the same package can be loaded within the same runtime environment. While it is unlikely that an application or package would intentionally load both versions directly, it is common for an application to load one version while a dependency of the application loads the other version. This hazard can happen because Node.js supports intermixing CommonJS and ES modules, and can lead to unexpected behavior.

dburles commented

Here's a simple app (https://github.com/dburles/dph-graphql-live-query) that demonstrates how it's possible to import into memory and run two copies of the same function. The demo app is denoted as a "module" by the top level type field in package.json (See: https://nodejs.org/dist/latest-v16.x/docs/api/packages.html#determining-module-system), and .js files are interpreted as ESM.

The diff function from @n1ru4l/json-patch-plus is imported (and exported for the sake of the demo) in both demo.js and demo.cjs. The app will print false as the result of calling console.log(diffesm === diffcjs);.

I've also added an example of when bundling this application, both the ESM and commonjs source of @n1ru4l/json-patch-plus get included in bundle https://github.com/dburles/dph-graphql-live-query/blob/main/dist.js.

As stated above: "While it is unlikely that an application or package would intentionally load both versions directly (as in this demo -David), it is common for an application to load one version while a dependency of the application loads the other version."

  • 👍1