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

Convert to type "module" and use package exports #432

Merged
merged 2 commits into from
Jun 7, 2021
Merged

Convert to type "module" and use package exports #432

merged 2 commits into from
Jun 7, 2021

Conversation

lkmill
Copy link
Contributor

@lkmill lkmill commented Jun 2, 2021

This PR uses type: "module" and package exports to fully support ESM in modern versions of Node.

Closes #425

@stefnotch
Copy link
Collaborator

stefnotch commented Jun 2, 2021

I personally would prefer option one from the linked blog post. After all, gl-matrix is at the point where barely anything ever changes, which means that pointing people with old build tools to an older version of gl-matrix should be fine. We'd have to release a new major version of gl-matrix though. (updated the issue to reflect on this)

Migrating
There are two ways to move your packages to ESM:

  1. Pure ESM
    This has the benefit that it’s easier to set up. You just add "type": "module" to your package.json, require Node.js 12, update docs & code examples, and do a major release.
    -- Option one

@lkmill
Copy link
Contributor Author

lkmill commented Jun 2, 2021

This is completely fine by me. The only caveat is that this not only requires newer versions of node, but to also be running Node with ESM files. Since TypeScript currently doesn't support outputting Node compatible ESM files (they do not include file extensions in import calls) the new gl-matrix wont be consumable through typescript in Node. This should not be a problem in frontend code though, which is the main use case of gl-matrix.

@stefnotch
Copy link
Collaborator

Good to know about that limitation. I suppose this should be fine, like you've said.

@lkmill
Copy link
Contributor Author

lkmill commented Jun 2, 2021

The project has now been converted to type: "module" and the CJS and nested package.json builds have been removed. I also converted all build and config files to ESM. I had to update rollup to support this fully.

@lkmill
Copy link
Contributor Author

lkmill commented Jun 2, 2021

The Travis build seems to be using Node 12.16.1, which does not support ESM modules. All tests pass locally (in ^12.17, ^13.2 and >= 14).

@lkmill lkmill changed the title Define package exports and use .mjs extension for ESM files Set type "module" and use package exports Jun 2, 2021
@lkmill lkmill changed the title Set type "module" and use package exports Convert to type "module" and use package exports Jun 2, 2021
@stefnotch
Copy link
Collaborator

Feel free to bump up the Node version used in Travis

- "12.16.1"

@lkmill
Copy link
Contributor Author

lkmill commented Jun 2, 2021

ok sorted, set it to latest LTS 14.17.0.

@lkmill
Copy link
Contributor Author

lkmill commented Jun 2, 2021

should the common file be exposed in a export? for something like import { toRadian } from 'gl-matrix/common'?

@stefnotch
Copy link
Collaborator

Yes please, do expose the common file. The toRadian and other methods are useful to have.

@lkmill
Copy link
Contributor Author

lkmill commented Jun 2, 2021

done! i see now common is exported as glMatrix from index.js, not common... should this be renamed, or would that be too much of a breaking change?

@stefnotch
Copy link
Collaborator

Oh, if it's already being exposed as glMatrix, we can keep it that way.

@lkmill
Copy link
Contributor Author

lkmill commented Jun 3, 2021

it's exposed in a little different way... today you can:

import { glMatrix } from 'gl-matrix'

glMatrix.toRadian()

with my change you can also:

import { toRadian } from 'gl-matrix/common'

toRadian()

i was wondering if i you think i should rename the named export glMatrix > common, rename the package export common > glMatrix or leave it.

@stefnotch
Copy link
Collaborator

I think it's currently fine with import { glMatrix } from 'gl-matrix'. If both options are now possible, that's also fine.

@lkmill
Copy link
Contributor Author

lkmill commented Jun 4, 2021

ok, then I'm feeling pretty done with this PR and it should be ready for merge!

- All util and config files have been converted to ESM
- Use file extensions in import calls spec files
- Use Node 14.17.0 in Travis
@stefnotch
Copy link
Collaborator

Thank you!

@stefnotch stefnotch merged commit c19b07f into toji:master Jun 7, 2021
@lkmill lkmill mentioned this pull request Jul 30, 2023
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.

Migrate to fully ESM
2 participants