Fix for importing Ramda in ESM-compatible nodes#2999
Conversation
| @@ -0,0 +1,6 @@ | |||
| const fs = require('fs'); | |||
There was a problem hiding this comment.
I've not done this in shell to support Windows contributors.
There was a problem hiding this comment.
I'm not on Windows, but I really appreciate this consideration. It helps reduce the barrier to entry for contributing.
| "build:es": "cross-env BABEL_ENV=es babel source --out-dir es", | ||
| "build:es": "cross-env BABEL_ENV=es babel source --out-dir es && node ./scripts/addModulePackageScope.js", | ||
| "build:cjs": "cross-env BABEL_ENV=cjs babel source --out-dir src", | ||
| "build:mjs": "cross-env BABEL_ENV=es babel source/index.js --out-file src/index.mjs", |
There was a problem hiding this comment.
I don't think this was particularly needed and should be even more obsolete thanks to the changes here.
| ".": { | ||
| "require": "./src/index.js", | ||
| "import": "./es/index.js", | ||
| "default": "./src/index.js" |
There was a problem hiding this comment.
"require" and "default" are serving the same purpose - CJS consumers, problem is that there was an experimental version of node@13 which had support for "import" but had no notion of the "require" key, so this "duplication" handles both versions at the same time as that experimental version of node can just fallback to the "default" whereas the new version will match against "require"
| "import": "./es/index.js", | ||
| "default": "./src/index.js" | ||
| }, | ||
| "./es/": "./es/", |
There was a problem hiding this comment.
Those are here to allow for "deep" imports like import add from "ramda/es/add.js". This isn't needed per se and you could explore hiding those files entirely through listing all allowed entries in this "exports" field (which could result in things being accessible like this: "ramda/add"), but I've added this for backward compatibility for now.
| "bench": "node scripts/benchRunner", | ||
| "bookmarklet": "node scripts/bookmarklet", | ||
| "build:es": "cross-env BABEL_ENV=es babel source --out-dir es", | ||
| "build:es": "cross-env BABEL_ENV=es babel source --out-dir es && node ./scripts/addModulePackageScope.js", |
There was a problem hiding this comment.
the type of interpretation is set based on the nearest package.json file, so we can just add a package.json with {"type": "module"} to a directory to make files in that directory being treated as modules even if the root is treated as commonjs (and still use .js for all files!)
|
This looks really good to me. I have one concern, and that is that the changes are fairly inscrutable without your fantastic comments. Somebody who isn't already well versed in Is this an issue? Should we look at hacking in some comments? |
Sure, I can write this down somewhere. Not sure though if package.json itself is the right place to do so 😅 If you want to keep it there I'm OK with that - just let me know where should I put it. |
|
I honestly don't know where we should put it Edit: on second thought, I suppose we can always just point to this PR in the release notes for the next version and it's all explained 😄 |
|
Anyone understand why GitHub is saying that some checks are incomplete, but that Travis is saying they are complete? One had failed before for what looked like spurious reasons. I restarted, and it looks fine on Travis, but GitHub is saying otherwise. Any ideas? Other than that, this looks great to me. I think I would rather point to this pull than try to hack comments into 🌿 And thank your once again @Andarist for coming to our rescue. I really should learn more about this material. |
|
@CrossEye Can we just merge anyway since we can still visually verify the CI completed successfully? |
|
@Bradcomp: I was planning on giving it two days in case someone who understood what happened with Travis pointed out some flaw I didn't understand. But yes, I think we can merge soon. |
|
If this causes any problem (hopefully not) then please just ping me and i will try to find a fix for it |
closes #2988
This all is quite new in general. node has added support for ESM just recently so keep in mind that I can't guarantee that this covers everything ideally, but I've done the research and some testing and I'm positive about this working correctly. If any problems arise from this - just ping me and I will prepare appropriate fixes (hopefully this won't be necessary though).