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

The simpler transform is semantically wrong #4

Closed
3cp opened this issue May 12, 2020 · 3 comments
Closed

The simpler transform is semantically wrong #4

3cp opened this issue May 12, 2020 · 3 comments
Labels
bug Something isn't working wontfix This will not be worked on

Comments

@3cp
Copy link

3cp commented May 12, 2020

I came to your interesting package in searching of a lighter esm to cjs transformer.

But I noticed the code is semantically wrong which can lead to logic error.

Your example

// input
import { resolve as resolvePath } from "path";
resolvePath("./hello")

// what i wanted
const {  resolve: resolvePath } = require("path");
resolvePath("./hello")

Translated the esm import to a simple variable, this is wrong in esm semantic. There is a reason why all babel/TS transforms esm import into verbose a_namespace.an_exported_name, because in esm standard, the import does not create a local JS variable, it's a read-only property on the namespace of that esm module.

Consider following proof, create two local files:
foo.mjs

let foo = 1;
function increaseFoo() {
  foo += 1;
}
export { foo, increaseFoo };

index.mjs

import { foo, increaseFoo } from './foo.mjs';
console.log('foo is ' + foo);
increaseFoo();
console.log('foo is ' + foo);

Then use nodejs v12 or above

node --experimental-modules index.mjs

It prints out:

foo is 1
foo is 2

You can see that foo inside index.mjs is not a local variable, otherwise it cannot be mutated.

@sidvishnoi sidvishnoi added the bug Something isn't working label May 13, 2020
@sidvishnoi
Copy link
Owner

sidvishnoi commented May 13, 2020

You're right. I didn't think of it this way. Thanks for reporting!
Unfortunately, I don't think I will fix this, as I don't use this project anymore and it has served me well the way I used it. PR welcome though. I'll add it in readme otherwise.
Edit: Documented in readme.

@3cp
Copy link
Author

3cp commented May 13, 2020

I understand, most code do not expose this kind of edge case.
BTW, any other alternative (of babel and tsc) you know of?

@sidvishnoi
Copy link
Owner

I understand,

Thank you 🙂

BTW, any other alternative (of babel and tsc) you know of?

Umm no. I started using ES modules in general (Node 12+ and Deno) or Rollup in most cases.

@sidvishnoi sidvishnoi added the wontfix This will not be worked on label May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants