Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Convert each plugin to a function from a class to an overriding class #459

Merged
merged 5 commits into from Apr 21, 2017
Merged

Convert each plugin to a function from a class to an overriding class #459

merged 5 commits into from Apr 21, 2017

Conversation

ghost
Copy link

@ghost ghost commented Apr 9, 2017

Q A
Bug fix? no
Breaking change? no
New feature? no
Deprecations? no
Spec compliancy? no
Tests added/pass? tests pass
Fixed tickets
License MIT

Instead of assigning functions to a parser instance, this creates a class with overridden methods.
The class is a function of the plugins (only "jsx", "flow", and "estree"), and this function is cached.

Based on my earlier comment: #393 (comment)
Like #393, this avoids fixing the indent inside each plugin, in order to have a smaller diff.

This will improve performance because we aren't assigning functions directly to the instance any more.
I ran the perf script again on my home machine just to be sure:

branch time
master 26.8 26.8 27.1 26.0 26.0
plugin_class 12.8 13.0 13.1 13.3 12.7

@hzoo
Copy link
Member

hzoo commented Apr 9, 2017

Hey @andy-ms! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

@codecov
Copy link

codecov bot commented Apr 9, 2017

Codecov Report

Merging #459 into master will decrease coverage by 1.28%.
The diff coverage is 97.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #459      +/-   ##
==========================================
- Coverage   98.26%   96.97%   -1.29%     
==========================================
  Files          20       20              
  Lines        3511     3469      -42     
  Branches      934      935       +1     
==========================================
- Hits         3450     3364      -86     
- Misses         22       55      +33     
- Partials       39       50      +11
Flag Coverage Δ
#babel ?
#babylon 96.97% <97.03%> (-0.04%) ⬇️
Impacted Files Coverage Δ
src/parser/index.js 100% <100%> (ø) ⬆️
src/plugins/jsx/index.js 97.71% <100%> (-0.03%) ⬇️
src/plugins/estree.js 99.17% <100%> (-0.09%) ⬇️
src/index.js 100% <100%> (ø) ⬆️
src/plugins/flow.js 97.01% <94.44%> (-1.2%) ⬇️
src/parser/expression.js 93.84% <0%> (-3.54%) ⬇️
src/tokenizer/state.js 97.22% <0%> (-2.78%) ⬇️
src/tokenizer/index.js 97.32% <0%> (-1.03%) ⬇️
src/parser/statement.js 98.15% <0%> (-0.93%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 344f070...c5fec5b. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 9, 2017

Codecov Report

Merging #459 into master will decrease coverage by 0.05%.
The diff coverage is 96.03%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #459      +/-   ##
=========================================
- Coverage   98.26%   98.2%   -0.06%     
=========================================
  Files          20      20              
  Lines        3506    3462      -44     
  Branches      934     935       +1     
=========================================
- Hits         3445    3400      -45     
- Misses         22      23       +1     
  Partials       39      39
Flag Coverage Δ
#babel 82.49% <66.66%> (+0.15%) ⬆️
#babylon 96.96% <94.17%> (-0.04%) ⬇️
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️
src/parser/index.js 100% <100%> (ø) ⬆️
src/plugins/jsx/index.js 97.71% <100%> (-0.03%) ⬇️
src/plugins/flow.js 97.99% <94.07%> (-0.21%) ⬇️
src/plugins/estree.js 99.17% <98.95%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ef4366...20a83d9. Read the comment docs.

@DanielRosenwasser
Copy link
Member

Hey @loganfsmyth @hzoo, looks like the tests are fixed. Can you take another look?

@STRML
Copy link

STRML commented Apr 19, 2017

Like #393, this avoids fixing the indent inside each plugin, in order to have a smaller diff.

I so wish this were a default, but you can view the diff via https://github.com/babel/babylon/pull/459/files?w=1 (note the ?w=1) to avoid highlighting whitespace diffs.

@ghost
Copy link
Author

ghost commented Apr 19, 2017

@STRML Thanks!

@hzoo hzoo requested a review from danez April 19, 2017 21:02
Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

lgtm!

src/index.js Outdated
}
parserClassCache[key] = cls;
}
return cls;
Copy link
Member

@hzoo hzoo Apr 19, 2017

Choose a reason for hiding this comment

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

extra space? return cls; guess we didn't have a rule for this, I guess we should land that prettier pr later

@danez
Copy link
Member

danez commented Apr 21, 2017

I ran my perf script and the results are impressive. (Up to 100% faster 🎉🎉🎉🎉)

Before

name                            run   
./fixtures/angular.js           502ms 
./fixtures/backbone.js          60ms  
./fixtures/ember.debug.js       1468ms
./fixtures/jquery.js            279ms 
./fixtures/react-with-addons.js 576ms 

after

name                            run  
./fixtures/angular.js           298ms
./fixtures/backbone.js          27ms 
./fixtures/ember.debug.js       779ms
./fixtures/jquery.js            141ms
./fixtures/react-with-addons.js 314ms

Thanks a lot

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants