Throw error for multiple exports default

[kaicataldo]

Fixes https://phabricator.babeljs.io/T7242

[codecov-io]

Current coverage is 87.70%

Merging #3518 into master will increase coverage by <.01%

1. File ...ommonjs/lib/index.js (not in diff) was modified. more
   • Misses 0
   • Partials 0
   • Hits +4

@@             master      #3518   diff @@
==========================================
  Files           193        193          
  Lines          9385       9389     +4   
  Methods        1068       1068          
  Messages          0          0          
  Branches       2162       2163     +1   
==========================================
+ Hits           8230       8234     +4   
  Misses         1155       1155          
  Partials          0          0          

Powered by Codecov. Last updated by dc525ed...562e229

[jmm]

Hello @kaicataldo, thanks for your interest in improving Babel! I added a comment to the issue -- we need a spec reference to establish what the correct behavior is here. If this were a spec violation I would expect it to be in the Static Semantics: Early Errors (in which case Babylon would be the place to address it), but I don't see it there. So if it's a runtime error we need a reference for that. Thanks!

[kaicataldo]

Thanks for the comment! I'll be up front - my experience reading the spec is limited at best at the moment, but I think this section references the error we're reporting in this PR. The relevant part seems to be the note at the bottom:

The duplicate ExportedNames rule implies that multiple export default ExportDeclaration items within a ModuleBody is a Syntax Error. Additional error conditions relating to conflicting or duplicate declarations are checked during module linking prior to evaluation of a Module. If any such errors are detected the Module is not evaluated.

Does that look right to you?

[kaicataldo]

Whoops, looks like you were expecting a reply on Phabricator. Cross-posting there.

[jmm]

Thanks @kaicataldo! I think that's probably the right reference, but I'd have to take a closer look sometime to make sure I understand. I said earlier that if it's an early error, Babylon is the place to address it, but I thought about it more after and I guess it's not that straightforward. I think that'd be the ideal when the error exists in input code, but that doesn't cover nodes inserted during transformation. It's conceivable to do both. I've only very quickly skimmed your PR, so I don't know in detail how it works or if it addresses inserted nodes, or duplicate exports in general or just export default specifically. If we're going to do this kind of checking it'd be nice wind up with a solution that covers everything, preferably in a generalized way, but that doesn't necessarily mean it couldn't be done incrementally.

I'll be up front - my experience reading the spec is limited at best at the moment

Even though I've read it quite a bit it's pretty gnarly and I often have to really work at understanding it. Looks like this morning I was looking in the wrong place and you did a better job of locating the relevant part :)

Again, thank you for contributing :) Please be patient as this is kind of a big PR (I know it's mostly tests though) and there may be things that require more urgent attention or have been waiting longer.

[kaicataldo]

Thanks! Happy to work with you to figure out a better solution - been interested in contributing for a while and this seemed like a good place to start :)

My change is a 5-liner (just to point the next person who looks at this at the right place - it's difficult to sort through all the files!): https://github.com/babel/babel/pull/3518/files#diff-d14797ce7c2385d5701ea1ef6b4da702R173. I added it there because it specifically checks for path.isExportDefaultDeclaration(). Could make sense to also have an error for duplicate exports and have a more specific error for duplicate exports default. Let me know what I can do to help land this.

[hzoo]

I'd be ok with landing this now since the tests would all have to change in the same way anyway - depending on how easy/long it would take to get a pr in babylon too

[kaicataldo]

So the tests would be the same and once we added throwing on duplicate named exports in Babylon we could remove the error throwing here? I'm happy to look into the Babylon change when I'm back from vacation next week. That sounds fine by me - whatever is best!

[kaicataldo]

Anything else I can do to land this?

[danez]

👍 When babylon support this case in future, we can then remove this, but for now this is the best solution :)

[kaicataldo]

I'd like to look at Babylon and see if I can make the change :)

[hzoo]

Thanks @kaicataldo 😺

[dlmanning]

I get that this was fixing a violation of the module spec, but shouldn't it have triggered a major version bump? It causes previously working code to stop working. If I've missed some policy statement on subject, please direct me.

[jmm]

Hi @dlmanning. That's a tricky subject. Fixing spec non-compliance is a type of bug fix. (An important thing to keep in mind is that Babel, or any JS parser, isn't a validator. Not suggesting this applies to you, but I think some people are in the habit of trying code snippets in Babel, or {{insert engine here}}, and if it works, interpreting that to mean the code is valid.) Any bug fix can break consumers relying on the buggy behavior. So just as we wouldn't release a major version for other types of bug fixes, we're probably not going to do it every time there's a spec compliance fix. (And major versions are not entirely "free", especially with an ecosystem as complex as Babel's.)

But there was some discussion before about issuing some kind of warning in advance of changes like this: #3225. And if there are ways to improve handling of situations like this that aren't too disruptive, we're open to it.

[dlmanning]

@jmm : No debate that it was a bug and needed to be fixed. Yes, this is a grayish area as regards semver. On the other hand, numbers are plentiful and cheap. This really did break some people's builds. It's nice for that to only happen when you're looking out for it is all.

[dlmanning]

@jmm : In lieu of adopting a different policy for spec fixes like this, might I suggest a warning somewhere that indicates to users of babel that babel will not treat spec correcting changes as deserving major version bumps? That way at least folks have been warned to look after their own house.

[jmm]

@dlmanning

No debate that it was a bug and needed to be fixed. Yes, this is a grayish area as regards semver. On the other hand, numbers are plentiful and cheap. This really did break some people's builds. It's nice for that to only happen when you're looking out for it is all.

Yeah, semver doesn't perfectly solve all of these kinds of issues. Numbers are cheap, but in practice it's not always that easy to release major versions. We're not eager to break anyones build or cause unnecessary pain. We're open if there are reasonable ways to mitigate that.

In lieu of adopting a different policy for spec fixes like this, might I suggest a warning somewhere that indicates to users of babel that babel will not treat spec correcting changes as deserving major version bumps? That way at least folks have been warned to look after their own house.

That sounds fine to me (and I wouldn't expect other maintainers to object), but I feel a bit skeptical that it'll help much. It's hard for me to imagine people paying much attention to it or keeping it in mind, and like I said this is just a variation on fixing bugs breaking some stuff in general. Maybe it would at least encourage people not to think of Babel as a validator like I mentioned. Where would you suggest putting it?

[hzoo]

Yeah its definetely a hard issue (gray area for semver) since all bug fixes are breaking just depends on what people are relying on.

We could try to do a staggered release like npm does. We release on a cadence (say every week) and then set the first release to a next dist-tag and then move to latest the next week.

[DrewML]

Another suggestion: Specifically for issues with Babel correcting spec compliance (not all bug fixes), there is an option to release a version with a warning to users that is spit to stderr, with the file/line/column, so it's clear that the next release will break them. That warning could also link to (future) docs that specify what Babel's policy is re: semver and spec compliance.