Add pure ES module build

[cherue]

Hello,
please add an additional ES module build. The source code is already almost an ES module, the two incompatibilities are imports not pointing at files (with file extensions) and using class fields.

Class fields are more or less final and can be left alone so the only change necessary for src/ to be an ES module are the imports. I tried rewriting them which works fine in Node v13 and higher but breaks all the tests because jest does not support it jet. They're working on it though.

In the meantime I suggest adding a third babel build using no preset and an additional plugin: ["@pika/babel-plugin-esm-import-rewrite", {addExtensions: true}]. Also pointing the module field to the new build in package.json.

Are you open to this? And if so do you want a pull request implementing this?

BTW this is a great library and especially the documentation is fantastic

[eemeli]

Maybe? I mean, there is already a separate browser build, so adding a Node ES module target would not be an undue burden on top of that. In particular, now that Node 13 includes support for the "exports" package.json field that'll allow the same import paths to be used in CJS & ESM contexts, as is already done via "browser".

I'm also happy to keep on transpiling code, rather than publishing sources directly. This allows using features like class properties in code. On that note, I think it might make sense to delay introducing this in a release until Node v14 is released in April, so that the Babel transpilation target can be set as node: '14.0' -- moving that number up later might be construed to count as a breaking change, and I'd rather avoid a major version update unless necessary. v13 will after all go out of maintenance already in June.

[eemeli]

This turned out not to require any additional build target; just a little bit of package.json config and two additional .mjs files for the yaml/types and yaml/util endpoints. Those then import the same sources as their corresponding CommonJS re-exporters. Later on, it might make sense to add a new build to target modern environments, but that's a separate issue.

I opted not to add ES module endpoints for the deprecated import paths, on account of those never having worked in the first place in a Node ES context.

[cherue]

Cool but this is Node only, so it's more of a "Node ES module". This does not add compatibility for Deno or quickjs. Should I make a separate issue for a "pure ES module" build?

Later on, it might make sense to add a new build to target modern environments, but that's a separate issue.

How do you determine what time that makes sense?

[eemeli]

Ah, I may have misunderstood what you were asking for earlier. No need to open a new issue, let's just reopen this and specify the title a bit more.

Providing an ES-only version of the library in parallel with the CommonJS export is tricky, because the AST level of its API uses and exposes a bunch of classes, which are then used for instanceof checks and otherwise. And a Node is only a Node if they both come from the same source, rather than separate files. The Node docs present these dual package hazards pretty well.

The current ES module implementation therefore relies on being able to import CommonJS files, which are the actual sources. Those then transitively require() other sources. Switching to using ES modules as the source of truth has its own problems, as it'd effectively mean dynamically import()ing from CommonJS, and the yaml API isn't promise-based. Horrible hacks might work around that, but they would indeed be horrible hacks.

I'm not really deeply familiar with Deno, but I gather that with something like this it'd be possible to add a require() to the environment for such use? I'm not aware of anything similar existing for QuickJS.

Going forward, I'd be quite willing to consider a v2 of the library that would drop the CommonJS export, and effectively solve this problem at the same time. But I don't think that makes sense quite yet, as almost all Node apps are still using CommonJS at least at runtime.

[qraynaud]

Maybe another way of fixing this would be using another method to check if an object is a Node. I'm not saying you should do that, since I don't know the whole codebase and its implications, but maybe something like that would work well:

const NODE_SYMBOL = Symbol.for('npm.yaml.Node')

class Node {
  static yamlType = NODE_SYMBOL
  static isNode(val) {
    return typeof val === 'object' && val?.constructor?.yamlType === NODE_SYMBOL
  }
}

And then, the official way of testing if something is a Node wouldn't be to use instanceof but Node.isNode() which can be easily documented. We have already similar problems for Promise and such, so it wouldn't be entirely new for the JS community.

[eemeli]

I'd really rather not break instanceof just as a workaround for dual-package hazards that could be caused by a change in the packaging in order to support environments that do not provide a global-scope CommonJS require().

Given that the actual source is entirely ES, it should be possible for anyone that needs an ES-only package to build it from the repo. Therefore, and given that I don't see this happening with the npm-distributed package at least for a year, possibly more, I'm going to close this issue (again). This should also clarify that this isn't expected to become a part of the yaml 2 release in a few months' time.