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

breaking: convert package to ESM #41

Closed
wants to merge 1 commit into from
Closed

Conversation

LinusU
Copy link

@LinusU LinusU commented Jul 24, 2021

This might be a bit premature since it quite aggressively cuts of older versions of Node.js, but I'm (and other maintainers) moving over my packages to ESM and would love a version 3.x of on-finished that has ESM support.


Migration Guide:

This relases changes the package from a Common JS module to an EcmaScript module, and drops support for older versions of Node.

  • The minimum version of Node.js supported is now: 12.20.0, 14.13.1, and 16.0.0
  • The package must now be imported using the native import syntax instead of with require

Migration Guide:

This relases changes the package from a Common JS module to an EcmaScript module, and drops support for older versions of Node.

- The minimum version of Node.js supported is now: `12.20.0`, `14.13.1`, and `16.0.0`
- The package must now be imported using the native `import` syntax instead of with `require`
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

The main issue to merging somelike this is that it means express and system cannot use it, and then there are two version lines to maintain. Best would be to have it such that it is both esm and commonjs at the same version.

@dougwilson dougwilson added the pr label Jul 24, 2021
@dougwilson
Copy link
Contributor

I can certainly appreciate that there is a set of maintainers who are aggressively moving towards ESM only, but I would classify myself along with the set of maintainers who more value wide version support, thus is why I would rather add ESM in a way that would not break CommonJS for the current time. I unfortunately have not used ESM at all yet and am not familiar with it, which of course can also be troublesome with this PR (merge the package to be something the main maintainer cannot use).

@dougwilson
Copy link
Contributor

I also realized, though, that this migration would actually be pointless. That required Node.js version range makes this package obsolete. What this module does is now part of Node.js itself, thankfully. If you are targeting only those version ranges, does thia package actually do something that you cannot do with the built in finished event?

@dougwilson
Copy link
Contributor

@LinusU
Copy link
Author

LinusU commented Jul 24, 2021

The main issue to merging somelike this is that it means express and system cannot use it, and then there are two version lines to maintain. Best would be to have it such that it is both esm and commonjs at the same version.

Yeah, I agree, I was mostly opening it since I a) wanted to see what effort it would take, and b) wanted to see if there was any interest. I knew upfront that it would probably not fit with the project 👌

I can certainly appreciate that there is a set of maintainers who are aggressively moving towards ESM only, but I would classify myself along with the set of maintainers who more value wide version support, thus is why I would rather add ESM in a way that would not break CommonJS for the current time.

I personally feel that shipping packages that support both ESM and CommonJS at the same time is not the way to go forward. I haven't investigated this too much, but as I see it you get a very little upside compared to just shipping CommonJS at that point, and a lot of drawbacks.

I also realized, though, that this migration would actually be pointless. That required Node.js version range makes this package obsolete. What this module does is now part of Node.js itself, thankfully. If you are targeting only those version ranges, does thia package actually do something that you cannot do with the built in finished event?

This is amazing! 🙌

And also this is in core now: https://nodejs.org/dist/latest-v16.x/docs/api/stream.html#stream_stream_finished_stream_options_callback

Awesome, I'll just use import { finished } from 'node:stream/promises' instead!

Thank you for taking the time to write out the detailed response! ❤️

I'll close this since I don't think it's in our interest to merge it...

@LinusU LinusU closed this Jul 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants