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

transform-module-commonjs' use of `var` vs. `const` #9435

Open
rdmurphy opened this Issue Jan 31, 2019 · 2 comments

Comments

Projects
None yet
3 participants
@rdmurphy
Copy link

rdmurphy commented Jan 31, 2019

Feature Request

Is your feature request related to a problem? Please describe.
Not necessarily a problem, but something I noticed that's different from how TypeScript's compiler handles the conversion of an ES module import to a CommonJS require — if browser support is flagged appropriately, TypeScript will use a const for this transform for anything ES2015 or above (via TypeScript's --target flag) :

// in TypeScript
import { join } from 'path';

join('hello', 'world');

// becomes

const path_1 = require('path');

path_1.join('hello', 'world');

But in Babel (via @babel/plugin-transform-module-commonjs), it'll always use a var due to how it's hardcoded in.

header = template.ast`
var ${metadata.name} = ${init};
`;

// in JavaScript + Babel
import { join } from 'path';

join('hello', 'world');

// becomes

var _path = require('path');

_path.join('hello', 'world');

Describe the solution you'd like
Maybe it's not a big deal. 😄 But did think it was interesting there was a difference! The perfectionist in me would prefer it used const when a compilation target allowed for it (and an imported value technically is readonly in an ESM environment). But anyway! Wanted to see if anyone else had feelings about it. I admit I don't know the technical challenges from Babel's perspective regarding this - is this is something @babel/plugin-transform-block-scoping would then catch later and correctly convert if necessary?

Describe alternatives you've considered
Only to not get so caught up about it. 😁

Teachability, Documentation, Adoption, Migration Strategy
I do think there's something to be said for using const all through your code, only to see the compiled version sneak a var in there (while leaving all the other consts alone) and how that could be confusing. But in general I feel like this could be a painless alteration unless transform-block-scoping wouldn't catch it.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Jan 31, 2019

Hey @rdmurphy! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community
that typically always has someone willing to help. You can sign-up here
for an invite.

@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented Feb 1, 2019

This comes down to a few things:

  • We don't have a great story around plugin ordering yet, so I'd definitely worry about it being missed
  • It's still relatively common to just compile to ES5, so it just hasn't been a priority
  • If we output const we then need to clearly document that we output ES6 and have some way to make it clear to users what plugins they'd need to then get ES5. Not that this is insurmountable, but it raises 1 big question for me. What features are we allowed to use, and what qualifies as a breaking change? If our for-of transform outputs code with const, and thus expects all users to have the block-scoping transform, and later we decide to have it output destructuring, e.g. {value, done} = iter.next(), would it count as a breaking change? It could certainly be breaking if the user had explicitly only enabled for-of and block-scoping transformation, and suddenly start getting destructuring in their output. Then we get into the territory of plugin dependency systems and such.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment