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 node.js to ESM #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

piranna
Copy link

@piranna piranna commented Dec 18, 2022

No description provided.

@BatiGencho
Copy link

@heineiuo Can you please merge this one in ?

@heineiuo
Copy link
Owner

Is there any reason to do this?

As upstream package (ws) has't changed type to module, I don't think it's a good timing to merge this.

@piranna
Copy link
Author

piranna commented Dec 30, 2022

I don't remember the exact details, but It has give me problems on hibrid environments, since browser import was ESM and Node.js one was CommonJS, correct aproach is to unify everything as ESM, as this PR does. After this change, everything is working flawlessly.

@piranna
Copy link
Author

piranna commented Jan 11, 2023

Any update on this? Can it be merged?

@heineiuo
Copy link
Owner

I'll keep this pr open until ws support type: module.

@piranna
Copy link
Author

piranna commented Jan 11, 2023

I'll keep this pr open until ws support type: module.

It will not happen in the foreseen: websockets/ws#1886.

I think we should NOT wait to ws doing the migration at all, and do it ourselves right now, to keep a consistent API: or both browser and Node.js support ESM, or none of them, leaving what ws currently does as an implementation detail. We can also offer both ESM and CommonJS exports for both browser and Node.js, or at least for Node.js since for browser we already migrated to ESM and deprecated to CommonJS. Would this make sense? Would you merge the PR sooner if I add support for both ESM and CommonJS, using package.json exports field?

@jost-s
Copy link

jost-s commented Apr 24, 2023

I don't understand why, but when using isomorphic-ws with Web Dev Server, it only compiles successfully when converting node.js to ESM. Here's reproduction for it https://github.com/jost-s/wds-error-reproduction

I've raised the issue in the WDS repo too modernweb-dev/web#1700 (comment)

@monsterbitar
Copy link

@heineiuo consider this solution as an alternative that you can apply today: #30 (comment)

Effectively keeping this as CJS, but exporting an equivalent ESM version separately for those that need it.

@piranna
Copy link
Author

piranna commented Jun 16, 2023

@heineiuo consider this solution as an alternative that you can apply today: #30 (comment)

Effectively keeping this as CJS, but exporting an equivalent ESM version separately for those that need it.

I like it, can you do a PR?

@monsterbitar
Copy link

@heineiuo consider this solution as an alternative that you can apply today: #30 (comment)
Effectively keeping this as CJS, but exporting an equivalent ESM version separately for those that need it.

I like it, can you do a PR?

#37

@JoCat
Copy link

JoCat commented Jun 16, 2023

@heineiuo consider this solution as an alternative that you can apply today: #30 (comment)

Effectively keeping this as CJS, but exporting an equivalent ESM version separately for those that need it.

In general, this should work better in terms of backward compatibility

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.

6 participants