alumna / reflect

Reflect the contents of one directory to another. At the speed of light. :zap:
MIT License
16 stars 1 forks source link

Broken ES Module in version 1.1.0 #26

Closed adrian-branescu closed 9 months ago

adrian-branescu commented 9 months ago

There is a contradiction in the version 1.1.0 of the package. You state that the entry-point of this package should be treated as an ES Module by setting:

"type": "module"

in package.json.

You also set the entry-point of this package to a CommonJS file:

"main": "dist/reflect.cjs.js"

in package.json.

The end result is that when you import @alumna/reflect you get a broken ES Module. Also, if you try to require it, the Node.js module loader complains:

reflect.cjs.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.

The right solution is to use conditional exports with entries for import and require.

paulocoghi commented 9 months ago

Thanks, Adrian! Solving it right now :zap:

paulocoghi commented 9 months ago

Fix released on v1.1.1. @adrian-branescu, please, when possible, check if it is working again now.

adrian-branescu commented 9 months ago

Fix released on v1.1.1. @adrian-branescu, please, when possible, check if it is working again now.

It complains about the path you've used:

Error [ERR_INVALID_PACKAGE_TARGET]: Invalid "exports" main target "dist/reflect.es.js" defined in the package config /data/workspace/warpos/build.engage-py.core/node_modules/@alumna/reflect/package.json imported from /data/workspace/warpos/build.engage-py.core/repl; targets must start with "./"

I've locally edited the package.json and it seems ok after adding the "./" prefix to the paths.

Also, for the CommonJS version to work, I think you have to use .cjs extension instead of .cjs.js because it still complains:

reflect.cjs.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.
Instead rename reflect.cjs.js to end in .cjs
paulocoghi commented 9 months ago

Ok, fixing.

paulocoghi commented 9 months ago

Sorry for the delay and thanks for the excellent references! I read more carefully the recommendations for "Dual CommonJS/ES module packages" (the next section after the one you linked) and I learned a lot from both sections.

When possible, I cordially ask you to test v1.1.3

paulocoghi commented 9 months ago

After testing all possibilities, I concluded that the most backwards-compatible strategy is

{
    "exports": {
        "import": "./dist/reflect.mjs",
        "require": "./dist/reflect.cjs",
        "default": "./dist/reflect.mjs"
    },
    "main": "dist/reflect.mjs",
    "module": "dist/reflect.mjs",
    "type": "module",
}

And the rationale is:

This is finally available in v1.1.3 :tada: :smile: