Add no-mutable-exports

[josh]

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]

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)

[jfmengels]

For consistency, could you wrap const in quotes too?

[jfmengels]

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

[jfmengels]

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]

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

[jfmengels]

@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]

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]

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]

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]

@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.

[josh]

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

[lukeapage]

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

[lukeapage]

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]

@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]

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

Ha, is "pass" an acceptable answer?

[lukeapage]

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]

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]

Doesn't merge anymore :(

[benmosher]

Rebased and merged! 😄

[benmosher]

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]

Ive been involved in lots of projects and no its not unusual.

[jfmengels]

@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]

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.