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

ES Module conversion #92

Closed
wants to merge 8 commits into from
Closed

ES Module conversion #92

wants to merge 8 commits into from

Conversation

markcellus
Copy link

@markcellus markcellus commented Sep 21, 2019

This converts the entire package to an ES Module and adds TypeScript support. ES Modules are the way forward and gives consumers a ton of benefits.

I realize that this may be a little too modern for many consumers who use very older versions of node, but we can possibly release this as an alpha release first to allow consumers to test it out before fully committing.

Please let me know your thoughts!

Fixes #87

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.

As long as this can be done without dropping support for so many Node.js versions. This PR doesn't even support all the active LTS versions of Node.js, which is 8.x (https://nodejs.org/en/about/releases/)

@markcellus
Copy link
Author

markcellus commented Sep 21, 2019

Maintenance for node 8 will end by next year, until then, we should release under alpha/beta versions only. The latest stable version of node is 10.

@dougwilson
Copy link
Contributor

Sure, but I was just using that as an example. I use this module myself down to Node.js 0.10. So accepting a PR that doesn't at least support down to there means I cannot even use my own module, which seems backwards. You're always welcome to publish your own module to support whatever Node.js versions you will use it on, though.

Let me know if you are interested in bringing back support for those old versions, otherwise we can probably close this as won't fix.

@markcellus
Copy link
Author

No problem! I'll close. Thanks for considering.

@markcellus markcellus closed this Sep 21, 2019
@dougwilson
Copy link
Contributor

Thanks for the PR, of course. I'm always happy to consider. It sucks if there is no way to support both the ES module folks and the CommonJS module folks, though. I don't mind adding features / whatever to the module as long as I can actually still use the module :)

@wesleytodd
Copy link
Member

It sucks if there is no way to support both the ES module folks and the CommonJS module folks

Yes, it sucks very much. I have tried multiple times, even once within the last month, to setup a easy to maintain system for supporting both, and right now I think the only reasonable way is to maintain a fork and publish two packages.

There are solutions out there trying to be better than this, but they are crazy complicated and I would not want to maintain a package using them.

@wesleytodd
Copy link
Member

wesleytodd commented Sep 21, 2019

@mkay581 I just saw your other comment and it looks like this is the approach you are taking. I think that it should not be too difficult to land any patches that land here on your fork. So that might help others looking for this support, thanks!

@markcellus
Copy link
Author

markcellus commented Sep 21, 2019

Thanks @wesleytodd! Yes, unfortunately we need to fork this. I successfully got cookie-esm working in the 1.0 version I just released. Please feel free to redirect any ESM inquiries over to that repository.

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

Successfully merging this pull request may close these issues.

Consider to support ES "module"
3 participants