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

Fix importing from modules #36

Closed
wants to merge 3 commits into from
Closed

Conversation

KTibow
Copy link
Contributor

@KTibow KTibow commented Feb 23, 2023

No description provided.

@KTibow KTibow mentioned this pull request Feb 23, 2023
@extremeheat
Copy link
Member

What does this fix? Can you provide example code on replit/runkit that fails before and passes now?

It seems to me like you are not changing anything minus deleting the .mjs wrapper. There is no difference in JS doing

module.exports = v
v.Vec3 = Vec3

and

module.exports = v
module.exports.Vec3 = Vec3

as they are both objects pointing to the same thing.

@KTibow
Copy link
Contributor Author

KTibow commented Feb 23, 2023

In fact there is. I can't find the blog post that said this, but I tested this on my own machine and this lets import { Vec3 } from "vec3" work.

@extremeheat
Copy link
Member

Please provide an example on repit/runkit or similar tool that demonstrate what changes. This doesn't seem right to me.

@KTibow
Copy link
Contributor Author

KTibow commented Feb 23, 2023

Screencast.from.2023-02-23.09-21-14.webm

@KTibow
Copy link
Contributor Author

KTibow commented Feb 23, 2023

@extremeheat
Copy link
Member

extremeheat commented Feb 23, 2023

I looked into the source code and see that this interop was a recent change to Node.js, merged in 2020.

Seems like a giant hack to me, but worthwhile for the sake of compatibility. Naturally there is a large percent of modules that wouldn't work with this compatibility feature. nodejs/node#35249

As written there by @guybedford :

Specifically on that point, users writing CommonJS shouldn't be "designing" their CommonJS to support this detection. In those cases they should rather just ship an ES module wrapper. What we are more concerned about is how users import CommonJS from ESM and their understanding of this process, and there specifically for CommonJS projects that aren't necessarily well-maintained or being updated with new ESM support.

So seems to me there is no reason to merge this to make it work with that compat hack given the current master that already has a proper module wrapper.

@KTibow KTibow closed this Feb 23, 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.

None yet

2 participants