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

feat!: distribute as ESM #449

Closed
wants to merge 5 commits into from
Closed

feat!: distribute as ESM #449

wants to merge 5 commits into from

Conversation

Skn0tt
Copy link
Contributor

@Skn0tt Skn0tt commented Dec 6, 2023

Addresses #447 by moving this package to ESM.

Because Ava doesn't play super nice with ESM, i've swapped it out for Vitest.

@Skn0tt Skn0tt self-assigned this Dec 6, 2023
@Skn0tt Skn0tt requested a review from a team as a code owner December 6, 2023 10:25
@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 6, 2023

I'm pretty sure that the error reported in #447 is due to some TypeScript bug - but moving this package to ESM can't hurt, either way.

Considering that all supported Node.js versions have support for ESM, I don't think we need to cut a major release for this.

@Skn0tt Skn0tt requested a review from ascorbic December 6, 2023 14:13
@@ -13,12 +13,12 @@ jobs:
strategy:
matrix:
os: [ubuntu-latest, macOS-latest, windows-latest]
node-version: [14.0.0, '*']
node-version: [18.19.0, '*']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we still need to support Node 14 in our build shouldn't we keep it that way here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ESM support was still experimental in Node 14: https://nodejs.org/docs/v14.2.0/api/esm.html#esm_enabling

That's why the tests failed before I updated this here: https://github.com/netlify/functions/actions/runs/7113273604/job/19364886077

They became stable in Node 16, so maybe we can set the minimum version to Node 16?

Netlify Build doesn't have a runtime dependency on @netlify/functions, so it should be fine to update this independently. If we're intending to raise the minimum Build version anyway, we might as well start here! Especially since users can choose to stay on the old version of this package, until they upgraded their Node.js to v16+.

@eduardoboucas
Copy link
Member

@Skn0tt Are you planning on working on this? A PR with such a large diff will go stale very quickly.

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 14, 2023

I can work on it in the coming days. What's your feeling towards moving this to ESM in general, do you foresee any problems from that?

@eduardoboucas
Copy link
Member

I can work on it in the coming days. What's your feeling towards moving this to ESM in general, do you foresee any problems from that?

Any CJS functions that use any runtime components of this library (as opposed to just the types) would start failing, right? I think that would be acceptable as long as we cut a major version and make it clear in the release notes that the library is now ESM-only.

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 14, 2023

Yes, that's what i'm assuming. So i'll make it a major release then 👍

@Skn0tt Skn0tt changed the title feat: distribute as ESM, replace ava with vitest feat!: distribute as ESM Dec 14, 2023
@Skn0tt Skn0tt closed this Apr 10, 2024
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.

4 participants