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

Add no-mutable-exports #290

Closed
wants to merge 11 commits into from
Closed

Add no-mutable-exports #290

wants to merge 11 commits into from

Conversation

josh
Copy link
Contributor

@josh josh commented Apr 28, 2016

Hey @benmosher, this plugin is pretty rad 🤘.

I added a rule to prevent the use of export let and export var. ES module export bindings are supposed to be live which is a pretty interesting concept, but tricky to shim correctly. Most CJS or AMD transpilers don't cover these cases correctly. So I'm just trying to avoid them in our code base until native implementations are finally here. export const is usually what you want anyway.

http://www.2ality.com/2014/09/es6-modules-final.html
http://stackoverflow.com/questions/32558514/javascript-es6-export-const-vs-export-let

@benmosher
Copy link
Member

Thanks for this, great rule. Use case makes a lot of sense.

Can you add a reference to the README under "Helpful warnings" and add a note to the change log? (See recent closed rule PRs for an example of the latter)

'ExportNamedDeclaration': function (node) {
const kind = node.declaration.kind
if (kind === 'var' || kind === 'let') {
context.report(node, `Exporting mutable '${kind}' binding, use const instead.`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, could you wrap const in quotes too?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some odd indentation here, do you mind fixing it?

@jfmengels
Copy link
Collaborator

Thanks for the rule @josh! :)

Quick question (I don't think I have a project set up to test it): is this valid?

const foo = 2;
export foo;

If so, would be nice to track the type of variables and error when foo is declared as let or var.

@benmosher
Copy link
Member

@jfmengels is that valid syntax? Should be export { foo } or export default foo?

@jfmengels
Copy link
Collaborator

@jfmengels is that valid syntax?

That's my question exactly. Figured out I could test it on astexplorer, and it does not seem like valid syntax, so it's 👍 on that front.

@benmosher
Copy link
Member

benmosher commented Apr 28, 2016

To that end, would be nice-to-have if let x = 2; export { x } was reported (I'm assuming it isn't per this work) but maybe that can come as an update later?

Might not be too hard, though; could capture all the declarations in the Program node body as shadowing is irrelevant since exports are always at module scope, and that visitor should be executed before any ExportNamedDeclarations. (could handle ExportDefaultDeclarations of the form let x = 2; export default x that way as well). Plus declarations are hoisted so it ought to happen upfront regardless.

@jfmengels, what's your take on doing that pre- vs. post-merge of this? should handling non-inline declarations be handled before publishing this at all? (I think that's the primary question for when to merge, should be publish-ready before hitting master)

@jfmengels
Copy link
Collaborator

would be nice-to-have if let x = 2; export { x } was reported

If that is valid code-wise but invalid exports-wise, it should be covered.

If @josh is fine with working on this and handling this case, we should wait until all cases are covered.

I don't know how semver applies with adding new errors in existing rules. The further addition of this case would probably be akin to a bug fix in the case, so a patch, but if we can ship this all in one go, I'd much prefer it.

@josh Do you mind continuing to work on this?

@josh josh closed this Apr 28, 2016
@josh josh deleted the no-mutable-exports branch April 28, 2016 17:18
@josh josh restored the no-mutable-exports branch April 28, 2016 17:18
@josh josh reopened this Apr 28, 2016
@josh
Copy link
Contributor Author

josh commented Apr 28, 2016

Spec allows declaration to be null. IIRC it should be for export { x as y }.

Added test coverage for null declarations. I'm not sure my declaration tracing implementation is ideal. Any better ideas?

@benmosher
Copy link
Member

@josh tests look good so I'm cool with what you've done. I'm 0% familiar with the scope object, thus my suggestion. I think yours is likely more robust.

A few considerations still:

  • let|var x; export default x is still not tested. I think it makes sense to handle this since it's analogous (and externally indistinguishable from) let x; export { x as default }
  • export (default?) class x {} and export (default?) function x() {} should probably both be valid even though they can be reassigned. I think it would be good to add valid test cases for all four of these.

@benmosher benmosher added this to the next milestone Apr 29, 2016
@josh
Copy link
Contributor Author

josh commented Apr 29, 2016

@benmosher great suggestions! Updated the PR. Let me know if I still missed some cases.

@lukeapage
Copy link
Contributor

this is great! I was about to ask for it and now I don't have to. Looking forward to the next version.

@lukeapage
Copy link
Contributor

export (default?) class x {} and export (default?) function x() {} should probably both be valid even though they can be reassigned. I think it would be good to add valid test cases for all four of these.

It would be good if that was checked as well, e.g. in a no-export-mutate rule

@benmosher
Copy link
Member

@lukeapage actually, I hadn't thought about it before, but this rule could check AssignmentExpressions for a left-hand Identifier that has been exported as a class or function. That is probably the simplest way to do it, especially given that only code within the module can reassign the names*.

*I'm not 100% positive this is true, but IIRC it is.

@josh would you be up for implementing that? i.e. for

export class x {}
// later: (or earlier, given hoisting)
x = "whatever" // report this reassignment to `x`

// need same for:
export function y() {}
y = "foo, or bar, or baz, or..." // report this reassignment too

// and even
function z() {}
export { z as a }
z = function someOtherZ() {}

@josh
Copy link
Contributor Author

josh commented May 4, 2016

@josh would you be up for implementing that? i.e. for

Ha, is "pass" an acceptable answer? :trollface:

@lukeapage
Copy link
Contributor

Eslint does already have a no-func-assign and no-class-assign so you could also either reccomend those in connection with this or look at their source for inspiration..

@benmosher
Copy link
Member

Fair enough, on both counts.

I think I'll merge and save function and class identifier checks for later. Will just make a note in the doc.

@jfmengels
Copy link
Collaborator

Doesn't merge anymore :(

@benmosher benmosher closed this in 940b294 May 5, 2016
@benmosher
Copy link
Member

Rebased and merged! 😄

@benmosher
Copy link
Member

BTW am I committing a GitHub faux pas by rebasing and merging PRs? I noticed it doesn't mark the PR merged but I do my best to keep the original author metadata on the commits when moving them around.

@lukeapage
Copy link
Contributor

lukeapage commented May 5, 2016 via email

@jfmengels
Copy link
Collaborator

jfmengels commented May 5, 2016

@benmosher no it's fine IMO. When you simply want to fix something without having a second commit, I would do the opposite though: merge, and then rebase/fix, that way contributors (especially the new ones) can see their work as properly accepted and merged.

Lodash does it all the time, and does it almost all of the time to have a clean history without merge commits (might be less rebases now that GH has a button for this), but it helps that there's one main maintainer on it (like here), as it may force contributors to rebase (which you'd like to avoid as much as possibl obviously).

@josh
Copy link
Contributor Author

josh commented May 5, 2016

Rebased and merged! 😄

❤️

BTW am I committing a GitHub faux pas by rebasing and merging PRs? I noticed it doesn't mark the PR merged but I do my best to keep the original author metadata on the commits when moving them around.

If you use the Squash button UI on the PR it will close it correctly. Otherwise if your doing things by hand, just put Closes #123 in the message so you get a nice backtrack reference. Once you rebase it becomes a completely different git SHA. So theres no way to automatically trace that back to any specific branch or PR.

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

Successfully merging this pull request may close these issues.

4 participants