Add ES module build

[lewisl9029]

Describe the pull request

This PR adds a ES module entry point to the library at /esm/index.js in addition to the existing CommonJS entry point at /lib/index.js.

Why

Most new frontend tooling like esbuild, Vite, and most package CDNs these days are esm-first, i.e. CJS modules require extra work and helper functions to get converted to esm. Exposing an esm entry point saves some work for these tools and reduces code size due to removal of CJS conversion helpers.

How

I added a new .babelrc-esm config file that configures babel to output esm instead of cjs, and a build:esm script that makes use of the config to output to a /esm dir, and a new "modules" entry point in package.json to point to /esm/index.js for esm consumers.

Screenshots

N/A

As far as I can tell, this changes nothing about the existing CJS outputs and workflow, and only adds the ESM entry point on top. However, there is still a chance of breakage due to the fact that many tools resolve to the module entry point over main by default, so they'll automatically switch over to this new esm build once this is released. So if we wanted to be extra cautious, it may be worth releasing as a new major and noting this as a potential breaking change.

I've published a version with the changes in this PR here, and tested with my own projects. So far they seem to work identically as before.

Happy to iterate on this until you're happy with it. Cheers!

[HiDeoo]

Thanks a lot for your pull request, really great and thorough work. I'm totally on board with the changes and agree with your reasoning.

• Thanks for typing out this in-depth description, covers everything I could think of.
• I like the approach of not adding new dependencies or new tooling, but rather using the existing ones in the project.
• The published version over @lewisl9029/intro.js-react is a great idea, allowed me to quickly test it in various demo sandboxes that I have and it worked great.

So if we wanted to be extra cautious, it may be worth releasing as a new major and noting this as a potential breaking change.

I totally agree with that statement, specially considering the state of the ecosystem and how often we see packages having issues with the whole CJS/ESM thing. I think it's better to be safe than sorry.

The only thing I'm not 100% sure without further investigation are the package.json modifications and if we should also provides an pkg.exports field or not.

What I propose is the following:

• I still have various projects using intro.js-react and I'd like to test this new version in them before releasing it. I don't think from my early tests that there will be any issues, but I'd like to be sure.
• I would like to review the report from publint.dev and see if some recommendations should be applied or not.

I'll try to do all of this as soon as I can.

If you have any thoughts or remarks, please let me know.

[lewisl9029]

Ah I didn't know about publint, those recommendations look great!

I think if we're going to do a major, we might as well do a few more potentially breaking changes so we don't have to do another one later down the line:

• Add the exports field as it suggested and only expose the main index.js and maybe Step and Hint modules as the public API.
• Add type: module to package json and rename the CJS modules to .cjs instead of renaming the esm to .mjs, since those will be used more often in frontend projects
• Maybe remove the src dir from the files list to slim down the package size.

Happy to update the PR with those changes if you're onboard! :)

[lewisl9029]

Oh and it could be worth considering passing an explicit browserlist to preset-env to minimize the need for polyfills. If a user needs support for older browsers, they could always configure their bundlers to transpile the package themselves.

[HiDeoo]

Add the exports field as it suggested and only expose the main index.js

Definitely agree.

Add type: module to package json and rename the CJS modules to .cjs instead of renaming the esm to .mjs, since those will be used more often in frontend projects

I was about to say yes regarding this point and remembered that @markerikson talked a bit about ESM compat issues in Redux / RTK and also did a lot of research regarding this topic.

I found this recent PR (and also this comment) which I think could be a solid starting point considering all the tests and research that went into it, the webpack 4 issues, etc. Based on this, here is the configuration I suggest:

"main": "dist/cjs/index.js",
"module": "dist/es/index.js",
"types": "index.d.ts",
"exports": {
  "./package.json": "package.json",
  ".": {
    "types": "index.d.ts",
    "import": "dist/es/index.mjs",
    "default": "dist/cjs/index.cjs"
  }
},

Some comments regarding this configuration:

• I suggest removing the existing lib folder and the newly introduced esm one and consolidate everything in dist/cjs & dist/es for consistency.
• "module": "dist/es/index.js" is explicitely a .js file (for webpack 4 support) which is just a copy of the dist/es/index.mjs file (reference)
• In the exports field, the "types": "index.d.ts", has to be the first one as documented in https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing

Maybe remove the src dir from the files list to slim down the package size.

Good catch. I have no idea why we have this in the first place. Looking at the blame this dates back to the initial release, not sure what I was thinking at the time. I think we can safely remove it.

Oh and it could be worth considering passing an explicit browserlist to preset-env to minimize the need for polyfills.

Sounds like a good idea and I think this can be inlined directly in the babel config files, e.g. "targets": "> 0.25%, not dead". Did you have any specific browserlist query in mind?

Happy to update the PR with those changes if you're onboard! :)

First, do you have any thoughts or objections regarding my last suggested changes? Maybe I missed something? If not, I would be happy to review such changes if you're still willing to update the PR. Otherwise I'll get to them when I have some time.

Once the changes are in, I would still like to do a quick test in various projects that I have and after that I think we should be good to go and release a new version.

[markerikson]

@HiDeoo : hiya! Yeah, my own experience so far is that you should not use type: "module" in package.json, and you should use .mjs for ESM files. (Except in the case of Webpack 4, in which case you should have a top-level "module" field that still has a .js extension.)

Also, there's a really neat project called https://arethetypeswrong.github.io/ that will inspect your package and let you know if TS thinks the entry points are defined properly, and I have put together a prototype CLI tool that uses that library. See arethetypeswrong/arethetypeswrong.github.io#15 for discussion.

[HiDeoo]

@markerikson Thanks for chiming in, really appreciated. Great resource too with arethetypeswrong.github.io, didn't know about this one. Can't wait for the day we have a definitive and comprehensive guide to properly deal with all this.

[markerikson]

@HiDeoo I know... I've been begging for years for someone who actually knows what they're doing to write such a comprehensive guide.

And no one has yet :(

I'm going to end up writing a blog post soon about what I've learned, but it's going to be "here's a bunch of stuff I've figured out by trial and error and I think half of that is still wrong", and definitely not a comprehensive authoritative guide.

[HiDeoo]

I'm going to end up writing a blog post soon

Would definitely be a great addition to your dev blog and also a solid, even if not authoritative, help to the community considering the state of the ecosystem.

[lewisl9029]

Wow, thanks for the discussion @HiDeoo and @markerikson, I never realized how deep this rabbit hole goes, learning so much haha.

I just pushed an update and published a new version for testing at https://www.npmjs.com/package/@lewisl9029/intro.js-react (version 0.7.1-7). Also ran it through arethetypeswrong and results are here: https://arethetypeswrong.github.io/?p=%40lewisl9029%2Fintro.js-react%400.7.1-7

Everything looks good except for this one point:

Imports of "@lewisl9029/intro.js-react" under the node16 module resolution setting when the importing module is ESM (its extension is .mts or .mjs, or it has a .ts or .js extension and is in scope of a package.json that contains "type": "module") resolved to CJS types, but ESM implementations.

I believe this is because we're using the same index.d.ts declaration file for both the esm and cjs entry points, and due to the lack of type: module, it's being treated as a CJS module. When I first tried the package in my app, typechecking was broken with the following TypeScript error:

After I removed the declare module wrapper from index.d.ts though, typecheck seems to work fine, but the same warning is still there, so maybe this is safe to ignore?

A few other things worth noting:

• I consolidated the two babel configs into 1 .js config that branches based on env.
• I used last 2 versions, not dead as the browserlist query. Including > 0.2% seems to end up including IE11 in the list and results in a bunch of helpers being added. Without it the output is nice and clean. I think we can ask users to transpile the module themselves if they need IE11 support so we don't waste bytes for everybody else.
• Had to add this plugin to rewrite imports to include extensions for the mjs version to work properly.

Let me know if y'all have any other feedback/suggestions. Happy to update further.

[markerikson]

@lewisl9029: yeah. That "FalseCJS" warning is what I'm getting for all the Redux alpha packages right now.

Per discussion with Andrew Branch from the TS team, the "Right" answer here is that you're supposed to somehow compile your types twice, once as "CJS" and once as "ESM", and have two different typedefs files.

I have not yet figured out a good way to do that in practice. So, for now, I'm ignoring that warning. (That of course means there's a good chance something will break :) But there's only so many changes I can keep making to all this package configuration.)

[lewisl9029]

@markerikson I managed to find a setup that gets rid of the warning by adding another layer of conditions and copy pasting the index.d.ts to each subfolder with the appropriate .d.mts/.d.cts extensions:

  "exports": {
    "./package.json": "./package.json",
    ".": {
      "import": {
        "types": "./dist/esm/index.d.mts",
        "default": "./dist/esm/index.mjs"
      },
      "default": {
        "types": "./dist/cjs/index.d.cts",
        "default": "./dist/cjs/index.cjs"
      }
    }
  },

Here are the new results: https://arethetypeswrong.github.io/?p=%40lewisl9029%2Fintro.js-react%400.7.1-10

So it seems like the warning was actually only complaining about the extension, and the actual content of the declarations file can stay in esm form for both (is there even such a thing as CommonJS-style declarations? I couldn't find any examples of anything other than esm-style declarations in the docs, where they used esm-style imports/exports even in declarations for cjs files: https://www.typescriptlang.org/docs/handbook/declaration-files/templates/module-d-ts.html).

For the case of this package and its handwritten declarations, all I had to do was copy paste it with the right extension. If you're generating the declarations with tsc it might get a bit more complicated. But just copy pasting the esm declarations to the cjs dir with .d.cts could still be worth a try? 🤔

[lewisl9029]

Oh hey I just found this example in the handbook that seems to match this format pretty closely:

https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing

    "exports": {
        ".": {
            // Entry-point for `import "my-package"` in ESM
            "import": {
                // Where TypeScript will look.
                "types": "./types/esm/index.d.ts",
                // Where Node.js will look.
                "default": "./esm/index.js"
            },
            // Entry-point for `require("my-package") in CJS
            "require": {
                // Where TypeScript will look.
                "types": "./types/commonjs/index.d.cts",
                // Where Node.js will look.
                "default": "./commonjs/index.cjs"
            },
        }
    },
    // Fall-back for older versions of TypeScript
    "types": "./types/index.d.ts",
    // CJS fall-back for older versions of Node.js
    "main": "./commonjs/index.cjs"

Hopefully that means it should be reasonably well supported from the typescript side at least.

[HiDeoo]

Fantastic investigation and changes @lewisl9029.

I have suggested a few changes as part of the pull request review. On top of that, here are some additional comments:

• I tested locally and can confirm your findings using the > 0.25%, not dead browserlist query. I am a bit hesitant on using last 2 versions, not dead as-is and I think > 0.5%, last 2 versions, Firefox ESR, not dead (which is basically the default query) may be a safer bet. Right now, it produces the same output as last 2 versions, not dead if I am not wrong (tested using git diff --no-prefix --no-index --binary dist dist-pr) but may be more future-proof? Any thoughts?

• I think it is safe to remove the ambient module (declare module) from the index.d.ts file like you did in the PR.

• I think we should explicitely add @babel/core as a development dependency. Got 90+ warnings about this unmet peer dependency when running a fresh install, a bit annoying.

Not a lot more to say, the PR looks more and more complete to me. I am looking forward to seeing the last few changes implemented, do some more testing on existing projects and get this merged and released. Thanks again for your work on this!

[lewisl9029]

Thanks for catching those missed updates and for the great feedback! The new browserlist config looks great, I'm happy as long as the output doesn't waste bytes for anything that's implemented in basically every browser outside of IE haha...

I've updated the PR with all of the requested changes. Let me know if you find anything else.

[markerikson]

@lewisl9029 :

I managed to find a setup that gets rid of the warning by adding another layer of conditions and copy pasting the index.d.ts to each subfolder with the appropriate .d.mts/.d.cts extensions:

Yeah, that's theoretically an option here. Andrew Branch from the TS team would likely tell you that there could be subtle differences in the generated typedefs depending on whether you're treating it as an ESM module or a CJS module, thus the need to run tsc twice. In practice, hopefully that's not a concern and you can get away with copy-pasting the file.

[HiDeoo]

I've updated the PR with all of the requested changes. Let me know if you find anything else.

This looks great. I've slightly updated the Babel config for tests as Jest didn't like the consolidated new one as-is.

I'll start testing this new version in a few projects for a few days and I think we should be good. Do you mind updating @lewisl9029/intro.js-react with the recent changes? It would make testing a bit easier for me but if not possible, that is not an issue.

[lewisl9029]

Sure thing, just updated the package with the latest changes at version 0.7.1-12. Let me know if you find anything!

[HiDeoo]

Sure thing, just updated the package with the latest changes at version 0.7.1-12. Let me know if you find anything!

Well, all my tests in a few projects went well, did not hit any issues, mostly drop-in replacements. I think we, and especially you, did everything to make the change as non-breaking as possible with all the informations we had.

I will merge this PR right now, and when I get home with a bit of free time, update the changelog with some warnings and publish a new major version 1.0.0.

Thanks again for you amazing work and patience on this PR. I'll make sure to ping this thread when the new version is released.

[HiDeoo]

🎉 v1.0.0 has been published to the npm registry. Thanks again for your work on this PR.

[lewisl9029]

Thank you for the review and great suggestions! I learned a ton. :)