Make packaging compatible with Node.js

[benmccann]

Fixes #16439
Fixes #18514
Fixes #18075
Fixes #18677
Maybe #18237 as well

The current packaging is not compatible with Node.js. All files will be treated as CommonJS unless "type": "module" has been set or the files end with .mjs. Adding "type": "module" was proposed in #18720. While I'd prefer that solution since I'm all in on ESM, it's a breaking change and probably better to do in a major version. This is the safe way to fix it

[SpBills]

Any update on this?

[robmadole]

This is a large change that we'll have to be careful with. So no ETA yet but it's on our radar.

[josdejong]

Thanks for working out this PR, I hope this issue will be resolved soon, and this approach of naming the files .mjs sounds like the proper solution.

Maybe a bit off topic, but I'm wondering if this isn't something that can/should be addressed in Vite. Looking in the package.json of Fortawesome, I see there is a "module": "index.es.js" field defined. I thought that the module field per definition should be an ES module file and treated as such?

[benmccann]

It's not Vite that has a problem with the names, but Node.js. If you use a bundler, you can bundle the files into a proper package. Vite does not bundle external dependencies by default and just lets Node run them directly. The problem here is that this package is not compatible with Node.js

[benmccann]

@robmadole @tagliala I'm happy to rebase this PR when you're ready to merge it. I'm not going to try to keep it up-to-date until you give the go ahead as that's a near impossible task. I'd really like to work with you to get this in. It's now the most upvoted PR by a factor of 2x: https://github.com/FortAwesome/Font-Awesome/pulls?q=is%3Apr+is%3Aopen+sort%3Areactions-%2B1-desc

Can you share if there are certain concerns you have or what testing you'd like to do to gain confidence that this change will be transparent to users? Or is there a v7 on the horizon that this could be included in if you still have some type of concern?

[josdejong]

It's not Vite that has a problem with the names, but Node.js. If you use a bundler, you can bundle the files into a proper package. Vite does not bundle external dependencies by default and just lets Node run them directly. The problem here is that this package is not compatible with Node.js

aaah, I get it now. Thanks for explaining and working out this fix!

[robmadole]

@benmccann thanks for doing this and your willingness to help.

The issue is purely one of breaking someone's existing build or project. So we have to be confident enough that whatever changes we make won't do that or that the percentage of folks who might be broken is low enough to be acceptable. For example, if it breaks Webpack 1.0 is that OK?

The other challenge for me on this is that the NPM packages pre-date Node's move to .mjs. We've been following "best practices" with regard to our packages from the beginning and it always feels like a losing battle. That we'll make some careful change that will break some weird edge case and frustrate users who are just trying to put some icons in their project. We hate that. Our goal is to save time not spend it.

So let me push back a bit and see if I can learn here.

Our @fortawesome/free-solid-svg-icons package has the following older attributes:

  "main": "index.js",
  "module": "index.es.js",
  "jsnext:main": "index.es.js",

These were best practice at the time they got added.

We then added this for tree-shaking (I don't think this has anything to do with the packaging issues though but I could be wrong.)

  "sideEffects": false,

And recently we updated to the new exports field:

  "exports": {
    ".": {
      "import": "./index.es.js",
      "require": "./index.js",
      "default": "./index.js"
    },
    "./index": {
      "import": "./index.es.js",
      "require": "./index.js",
      "default": "./index.js"
    },
    "./index.js": {
      "import": "./index.es.js",
      "require": "./index.js",
      "default": "./index.js"
    },
    "./package.json": "./package.json",
    "./*": "./*.js"
  },

So now we also need to rename files that are es.js to mjs? Why? Is this an actual packaging rule? Is this just some convention that has evolved over time?

What I can see in Node's docs is:

Enabling#
Node.js has two module systems: CommonJS modules and ECMAScript modules.
Authors can tell Node.js to use the ECMAScript modules loader via the .mjs file extension, the package.json "type" field, or the --input-type flag. Outside of those cases, Node.js will use the CommonJS module loader. See Determining module system for more details.

This makes sense. But I've also told Node what type of file it is through exports and module/jsnext:main. How many times do I need to tell Node what this file is? And the next spec/convention where now Node decides that files need to be in a directory called modules and they must be named with only lowercase vowels and never contain any browser objects like window or it will raise a ShitYouMessedUp error when running npm audit. I'm venting. This stuff is as frustrating for us here as it is for folks using it. (Ok, venting done).

So how do we proceed? Aside from trying to setup dozens of different projects (next, nuxt, vue, react, angular, ember, webpack, esbuild, require.js, etc.) and testing if a .mjs changes breaks stuff how can we gain confidence? Is there another project folks know of that has already been through this transition and we can curb off their experience? If I can follow their thread and though process and there is evidence that their collective world didn't break with the change I think I'd be all for this. But I will not break (intentionally or through lack of due diligence) folks who are already using our packages to support tooling that doesn't use one of the existing mechanisms for determining if a file is an ESM or not.

[ZetiMente]

Hi @robmadole

I would like to politely remind you of the other side of the coin. This has now broken my project and I'm now a frustrated user. But would like to ensure you that many projects have done this transition and @benmccann is constantly going to different projects and doing this exact same thing.

And I'm not alone: https://stackoverflow.com/questions/73081109/getting-error-when-importing-fontawesome-icons

[rdela]

@robmadole Completely empathize with your frustration and the difficulty of maintaining a project as popular as this one in the ever-shifting sands of Node.js, JS, and the web. As Ben wrote, this PR (renaming these 5 files to end in .mjs instead of .es.js and updating the 4 package.json files that reference them) is the way to resolve these issues in a non-breaking way. Adding "type": "module" to the 4 package.json files is an alternative approach to resolve the issues, but would be a breaking change.

Here are some examples I could find of projects that have been through the transition or contemplated similar changes:

• strapi-sdk-js: commit, PR:

  It seems that for some people the es.js extension doesn't work & the package is not recognised as an ES module

• eta: issue, PR, they went the "type": "module" route
• ractive: stuck with .mjs in 2017 and turned out to have predicted where things were heading correctly
• spacetime: eventually shipped .mjs in 2019 after a discussion that began in 2017
• microbundle: commit, PR, now outputs .mjs after a 2017 Twitter discussion with Tierney Cyren (@bnb)

Tierney has a highly informative series of articles, ESM in Node.js (also on dev.to) where they go into detail about the different options and how each option interacts with Node.

@ljharb's comment in #16439 gets at the technical reason .mjs emerged as the best solution in Node.js, as I understand it:

File extensions are only at the end - .es.js is just .js, and thus is not reliably distinguishable from any other .js file.

It's not easy to find examples because .es.js was never that widely adopted, although the Rollup community seems to have embraced it the most. Perhaps Tierney can elaborate.

Hope this helps add more context.

[benmccann]

@robmadole I share your frustration with Node.js here! That being said, I don't make the rules and just have to follow them. I've been helping across lots of different packages at this point, so have learned the rules pretty well and am happy to share

So now we also need to rename files that are es.js to mjs? Why? Is this an actual packaging rule? Is this just some convention that has evolved over time?

Node.js will load a JS file as CommonJS unless the package has "type": "module" or the .mjs extension, but I see you've already found those docs. I'd call it the Node.js spec as it's explicitly documented in the Node docs and many projects don't work without it.

But I've also told Node what type of file it is through exports and module/jsnext:main. How many times do I need to tell Node what this file is?

Node doesn't do anything with module/jsnext:main. It's a convention that was created by some bundlers and that many bundlers today will understand - although recent versions of all bundlers I'm familiar with will ignore these fields since you have defined exports. However, it doesn't hurt to leave it and may help some ancient bundler versions that don't understand exports since it's newer. The import under exports means that when you do an import() in Node it will use that file. Note that in Node.js, you can import() CJS files. For reference, the docs say: "Applies regardless of the module format of the target file." That's why Node doesn't know how to interpret the file without the proper file extension. Now I would be in full agreement with you in thinking that there's some stupidity in the way Node has done this, but I'm not sure there's much I can do about that.

So how do we proceed?

Hopefully by merging this PR! 😆 More seriously, we shouldn't have to test various JS frameworks (next, nuxt, vue, react, angular, ember) as they're not runtimes that execute the JS and are not bundlers that parse and compile the JS (though most do use bundlers, there's less permutations if we just consider the bundlers directly). The bundlers (webpack, esbuild) and loaders (require.js, etc.) do care about packaging changes in general. Although, I can say from experience, they just take the entry point and use it without regard to what the file extension is.

Is there another project folks know of that has already been through this transition and we can curb off their experience?

Yes! I've made this change in dozens of projects. I'm afraid I can't recall many of them as it's been at least a few months since the last one. But that's also good in a way because they were all non-eventful - if it were painful I'd be more likely to remember 😄 Searching through my inbox returned mostly unrelated results, but I did manage to find that Three.js added a .cjs extension (mrdoob/three.js#23325). I've come across most of these as a maintainer of SvelteKit where people report various packages not working with SvelteKit. We got a bit tired of seeing Svelte components that weren't packaged correctly, so wrote our own packaging tool. It's been released for several months and folks haven't hit the types of breaking issues we're talking about when using it, so I also know that these practices work well from that experience. We've also written an entry in our FAQ that seems to work quite well as we've pointed a number of people to it and haven't gotten any reports of problems following the advice there: https://kit.svelte.dev/faq#packages

tooling that doesn't use one of the existing mechanisms for determining if a file is an ESM or not.

Just to be clear, the tooling we're talking about here is Node.js! They're pretty much the largest JS runtime out there besides Chrome. I can reproduce this error just running node test.js without any other tooling involved. There are a lot of other tools out there that had created their own crazy stuff like jsnext:main before any kind of standardization had happened. I think if we're going to say we're not going to support non-standard conventions we can drop stuff like that and just follow the Node spec, which had been basically universally adopted.

long-term suggestion

This whole mess is caused by the JS ecosystem historically having had so many different file loading formats / conventions. Luckily this problem is starting to be resolved with the standardization of ESM. This problem only really goes away when everyone switches to ESM. When in the transition you want to do that is a project-by-project choice. But you could probably avoid a lot of complication by going ESM-only or at least ESM-first. By ESM-first I mean add "type": "module" to all packages and make the main field point to an ESM file and if you really need to distribute something like UMD for CDNs then define that with a browser field and fields like jsdelivr/unpkg for any others that require a special field. That's a breaking change though and would have to be done in a major release. It may cause some people some amount of trouble to update their projects, but that's work that I think will have to happen gradually across the ecosystem over time.

[robmadole]

Fantastic @rdela and @benmccann. So super helpful. I'm confident enough that we can try this then. I'll slot this for the next upcoming release. I appreciate your time and patience and the service to the community at large.

[benmccann]

Thanks @robmadole! Let me know when you're ready to merge and I can resolve the merge conflicts then.

[robmadole]

@benmccann we have another private repository (because it contains our commercial software) where these files get built. It's a simple enough change. Nothing additional needed from you I'll just double-check what you've done here as a reference.

[bnb]

@rdela I won't comment beyond this as I don't want to get in the way of y'all's work but just wanted to say I really appreciate how nice your comments here are to me. One of the kindest pings I've ever gotten on GitHub ❤️

[ljharb]

The simplest approach is to do CJS-only, since any node program that understands ESM can import CJS (or bundle it). The complexity arises when attempting to ship native ESM.

[mikestopcontinues]

@ljharb Somebody's gonna be first to ripping off the CJS bandaid. Better to be ready for it.

[ljharb]

@mikestopcontinues its already happened, and the result is a ton of user pain, and virtually no adoption. Do it at your own risk :-)

[benmccann]

This is primarily a client-side library. Distributing it only as CJS would be pure madness

ESM works far better with modern frontend development tools in my experience. It lets you skip using stuff like @rollup/plugin-commonjs and removes a layer of any potential bugs converting from CJS to something the browser can actually use and run

[ljharb]

You can only skip bundling if the rest of your app’s deps are published as native ESM, which is highly unlikely to ever be the case for most apps. I’m not aware of any inherent bugs caused by CJS/ESM conversion or bundling.

[robmadole]

OK! We've got this fixed and tested and ready to ship with 6.2.0. Thanks all for the patience and help.

[josdejong]

That is awesome news Rob, thanks! 🎉

[mikestopcontinues]

Thanks!

[ZetiMente]

@robmadole My project now builds with no hacky configurations ! Thank you !!!

[benmccann]

Yes! It's working for me as well now. Please feel free to close this PR and all the linked issues!

[robmadole]

Fantastic! Thanks all again.