Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compile to ES2015 modules instead of commonjs #373

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

justuswilhelm
Copy link
Contributor

Caveat: I have not tested whether this package will build correctly or not. I was only able to test it locally using npm run build and then using npm link ../$THIS_REPOSITORY bundling it into a vanilla-ts Vite test page.

Using ES modules (ESM) increases compatibility with modern tooling. Using commonjs regularly introduced issues when working various tools.

One of those issues was directly using this package with unpkg.com or the npm link command, e.g. npm link ../$THIS_REPO, resulting in build errors complaining about a default export missing (presumably since ES modules are expected).

Not being able to load this package using unpkg.com reduces testability, e.g., when wanting to try Sarus in a jsfiddle.

Furthermore:

It would be good to investigate at this point whether checking the dist/ folder into the repository makes sense at all. When building an NPM package, the dist folder can be packaged then, and since it can be derived using npm run build any time, it should be left out of the repository.

Being able to load this package using unpkg.com makes it easier for developers to evaluate whether this package is usable for their needs. Previously one needed to install it with NPM and use a build system that explicitly supports common js modules.

Every modern supported version of Node.js (the introducer of commonjs) supports ES modules as well. (See https://nodejs.org/api/esm.html)

Bundlers that support ESM:

Further note:

Consider switching back to >= ES2015 build target.

@paulbjensen
Copy link
Member

@justuswilhelm great suggestions, I agree it would be good to switch to ESmodule format.

The one library that I know could be used to check the build against is Hub - http://github.com/anephenix/hub. What I will do is try out this PR branch with Hub.

I agree that a GitHub action would be the ideal place to do the dist build, as well as to tag the version and publish it there. Eventually I would like to move towards that, I just need to find the time and focus to sit down and make that happen.

@justuswilhelm
Copy link
Contributor Author

Good :) Thank you. Please let me know if it works. I believe once this is figured out and documented nicely, it can be reused for other npm packages.

I found these two articles helpful:

I am also using npm link to test with es modules locally for this package. Otherwise I have troubles importing it into my vanilla vite ts project.

@paulbjensen
Copy link
Member

There are 2 more file changes required:

This line will need to be added into the package.json file:

  "type": "module",

And the jest.config.js will need to replace the module.exports = call with export default.

I tried to get it to work with Hub, but for that to work Hub would have to switch from using CommonJS to ESModule for the files. I'll spend a bit of time this weekend making that happen.

Additionally:
- Convert jest config to use ESM export
- Specify package as ESM in package.json ("type": "module")

Caveat: I have not tested whether this package will build correctly or
not. I was only able to test it locally using `npm run build` and then
using `npm link ../$THIS_REPOSITORY` bundling it into a vanilla-ts Vite
test page.

Using ES modules (ESM) increases compatibility with modern tooling.
Using commonjs regularly introduced issues when working various tools.

One of those issues was directly using this package with unpkg.com or
the `npm link` command, e.g. `npm link ../$THIS_REPO`, resulting in
build errors complaining about a default export missing (presumably
since ES modules are expected).

Not being able to load this package using unpkg.com reduces testability,
e.g., when wanting to try Sarus in a jsfiddle.

Furthermore:

It would be good to investigate at this point whether checking the dist/
folder into the repository makes sense at all. When building an NPM
package, the dist folder can be packaged then, and since it can be
derived using `npm run build` any time, it should be left out of the
repository.

Being able to load this package using unpkg.com makes it easier for
developers to evaluate whether this package is usable for their needs.
Previously one needed to install it with NPM and use a build system that
explicitly supports common js modules.

Every modern supported version of Node.js (the introducer of commonjs)
supports ES modules as well. (See https://nodejs.org/api/esm.html)

Bundlers that support ESM:
- Webpack: https://webpack.js.org/guides/ecma-script-modules/
- Rollup (used by Vite): https://rollupjs.org/es-module-syntax/

Further note:

Consider switching back to >= ES2015 build target.
- https://caniuse.com/websockets: 97.17%
- https://caniuse.com/es6: 97.15
Unless there is an argument to keep ES5, such as a specific client's use
case, it would be good to update. Furthermore, core-js has polyfills for
all es6 features: https://github.com/zloirock/core-js#ecmascript
@justuswilhelm
Copy link
Contributor Author

Thank you for the pointers. I have also taken the liberty to adjust the .editorconfig some more. Do you reckon further changes to package.json are necessary like described in the typescript documentation?

@paulbjensen
Copy link
Member

Hi @justuswilhelm ,

tried to get it to work with Hub, but for that to work Hub would have to switch from using CommonJS to ESModule for the files. I'll spend a bit of time this weekend making that happen.

I will work on that now.

@paulbjensen
Copy link
Member

Afternoon,

I will try and pick this up next for Sarus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants