Starcounter-Jack / JSON-Patch

Lean and mean Javascript implementation of the JSON-Patch standard (RFC 6902). Update JSON documents using delta patches.
MIT License
1.78k stars 215 forks source link

package.json should use `conditional exports`, else esm won't work inexplicitly #310

Open loynoir opened 1 year ago

loynoir commented 1 year ago

Actual

inexplicitly

import {compare} from 'fast-json-patch'
console.log({compare})
import {compare} from 'fast-json-patch'
        ^^^^^^^
SyntaxError: Named export 'compare' not found. The requested module 'fast-json-patch' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'fast-json-patch';
const {compare} = pkg;

explicitly

import {compare} from 'fast-json-patch/index.mjs'
console.log({compare})
Could not find a declaration file for module 'fast-json-patch/index.mjs'. 
'/path/to/node_modules/fast-json-patch/index.mjs' implicitly has an 'any' type.
If the 'fast-json-patch' package actually exposes this module, 
try adding a new declaration (.d.ts) file containing `declare module 'fast-json-patch/index.mjs'
{ compare: [Function: compare] }

Expected

import {compare} from 'fast-json-patch'
console.log({compare})
{ compare: [Function: compare] }
anonghuser commented 11 months ago

+1

To be more specific on what is needed, the following snippet should be added to package.json:

    "exports": {
        ".": {
            "import": "./index.mjs",
            "require": "./index.js"
        },
        "./index.mjs": "./index.mjs"
    },

This is the official NodeJS-supported equivalent of the "module" field which is an unofficial extension of package.json by tools such as webpack. It is documented here.

At least index.mjs should remain exported as a subpath, since it was documented in the README. Even if you remove it from the README, I think it should remain exported for backwards compatibility at least until a major version change. If you are aware people use other subpaths, i.e. require('fast-json-patch/commonjs/core.js'), either list them explicitly too or include a "./*":"./*" mapping instead of the explicitly listed index.mjs. If you want the .js extension to be optional as it is in the default require behavior so that require('fast-json-patch/commonjs/core') works, something like this could work:

        "./*.mjs": "./*.mjs",
        "./*.js": "./*.js",
        "./*": "./*.js"

There is also a provision for specifying the location of the type definition files or even full typescript sources in the new syntax instead of using undocumented fields at the root level, but I don't think typescript tools actually support it yet so it's a point for another time.

As for the Could not find a declaration file for module error mentioned in the issue when someone references a .mjs file, the .d.ts could be copied to .d.mts to resolve it. Perhaps it is pointless to do for index.mjs since there will be no need to use it after this update, but if you expect people to be referencing the modules/*.mjs files then rename their type definition files to .d.mts. This is somewhat tangential to the main issue though.