Force displayName plugin to run before all other transforms

[jamiebuilds]

Fixes gaearon/babel-plugin-react-transform#67

cc @gaearon

[jamiebuilds]

Better diff: https://github.com/babel/babel/pull/3209/files?w=0

[gaearon]

What if two transforms did this?
(Not trolling, just want to understand this a little better.)

[codecov-io]

Current coverage is 84.82%

Merging #3209 into master will decrease coverage by -0.29% as of e58f177

@@            master   #3209   diff @@
======================================
  Files          215     215       
  Stmts        15617   15619     +2
  Branches      3341    3341       
  Methods          0       0       
======================================
- Hit          13292   13249    -43
- Partial        679     724    +45
  Missed        1646    1646       

Review entire Coverage Diff as of e58f177

Powered by Codecov. Updated on successful CI builds.

[jamiebuilds]

@gaearon What is actually happening here is we're forcing this plugin to run in isolation.

Babel's transform pipeline optimizes the traversal process by merging visitors together. Basically making every visitor run at the same time. This is why order is (mostly) insignificant in Babel 6.

This sometimes causes issues with one transform "beating" another by hitting a node first and mutating nodes that another plugin wants to touch. What I've done here is hit the top-most node and forced an isolated traversal of the entire tree.

So to answer your question of "what if two transforms did this", it would just mean that the order is significant just like Babel 5. Since this transform is not really destructive in any kind of way it doesn't matter that much that it's running before other things.

[gaearon]

Thanks for explaining. 👍
Does this mean this would introduce a perf regression?

[jamiebuilds]

Does this mean this would introduce a perf regression?

Yes

[amasad]

Another option is for babel-plugin-react-transform to take on this plugin as a dependency and manually run the visitors before doing it's work.

[gaearon]

Wouldn't this result in duplicated work? Or can Babel detect that this happened?

[amasad]

I thought this plugin bails when it sees a displayName? But anyways it's a
trade off. IMHO it's better than everyone paying the cost whether or not
they use both plugins.

On Thursday, December 24, 2015, Dan Abramov notifications@github.com
wrote:

Wouldn't this result in duplicated work? Or can Babel detect that this
happened?

—
Reply to this email directly or view it on GitHub
#3209 (comment).

[gaearon]

IMHO it's better than everyone paying the cost

Yep, I agree.

[jamiebuilds]

I thought this plugin bails when it sees a displayName?

I mean technically it's going to revisit the nodes again, but thats pretty insignificant in comparison. I never thought to pull in the actual visitor in another plugin.

[sebmck]

This is bad, we shouldn't be encouraging plugins to be written like this. The gaearon/babel-plugin-react-transform issue doesn't actually explain why this is happening:

Basically since in Babel 6 plugin visitors get merged, this plugin will always run before babel-plugin-transform-react-display-name, meaning that displayName will never get added because we've already changed the AST.

What is causing this plugin to not be ran and why is this the only possible fix?

[sebmck]

Okay we can fix this by just crawling up the ancestry looking for appropriate parent nodes, breaking on statements so we don't go too high up. Alternate PR incoming.

[sebmck]

#3216