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.

Sign up for GitHub

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

Jump to bottom

Evaluate some String and Array instance methods at compile time #505

Merged
merged 11 commits into from
Jun 13, 2017
Merged

Evaluate some String and Array instance methods at compile time #505

merged 11 commits into from
Jun 13, 2017

Conversation

j-f1
Copy link
Contributor

@j-f1 j-f1 commented Apr 18, 2017

Ref #501.

@j-f1
Copy link
Contributor Author

j-f1 commented Apr 18, 2017

Is there a way to find out what prettier doesn’t like about my code?

@boopathi
Copy link
Member

Just run npm run format and it ll format the code. You can check the diff.

@j-f1
Copy link
Contributor Author

j-f1 commented Apr 18, 2017

@boopathi I’m still getting the error after running npm run format. What else should I do? It seems to work on my machine.

It could be because I’m spreads (f(...args) { /* code here */ }).

@codecov
Copy link

codecov bot commented Apr 18, 2017 •
edited

Codecov Report

Merging #505 into master will increase coverage by 0.48%.
The diff coverage is 91.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #505      +/-   ##
==========================================
+ Coverage   82.93%   83.42%   +0.48%     
==========================================
  Files          41       42       +1     
  Lines        2743     2860     +117     
  Branches      958     1003      +45     
==========================================
+ Hits         2275     2386     +111     
- Misses        282      283       +1     
- Partials      186      191       +5
Impacted Files Coverage Δ
.../babel-plugin-minify-constant-folding/src/index.js 85.56% <82.85%> (+3.02%) ⬆️
...plugin-minify-constant-folding/src/replacements.js 96.82% <96.82%> (ø)
...l-plugin-minify-dead-code-elimination/src/index.js 85.5% <0%> (ø) ⬆️
packages/babel-plugin-minify-simplify/src/index.js 80.74% <0%> (ø) ⬆️
...bel-plugin-minify-guarded-expressions/src/index.js 69.04% <0%> (ø) ⬆️
...gin-transform-inline-consecutive-adds/src/index.js 80% <0%> (+0.2%) ⬆️
...rm-inline-consecutive-adds/src/object-collapser.js 93.33% <0%> (+0.22%) ⬆️
...e-consecutive-adds/src/array-property-collapser.js 82.97% <0%> (+0.37%) ⬆️
...sform-inline-consecutive-adds/src/set-collapser.js 84.21% <0%> (+0.42%) ⬆️
... and 3 more

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 be3f3a7...6a17e4e. Read the comment docs.

@boopathi
Copy link
Member

Can you rebase with master ? I upgraded prettier today and fixed those things in master.

@j-f1
Copy link
Contributor Author

j-f1 commented Apr 18, 2017

@boopathi It looks like that didn’t fix the problem.

@boopathi
Copy link
Member

We can get to the formatting issue later. I'll review the PR.

@boopathi boopathi self-requested a review April 19, 2017 17:02
@boopathi boopathi added the Tag: New Feature Pull Request adding a new feature label Apr 19, 2017
boopathi
boopathi reviewed May 9, 2017
View reviewed changes
packages/babel-plugin-minify-constant-folding/src/replacements.js Outdated
const other = Symbol("other");

module.exports = ({ types: t }) => {
const undef = t.identifier("undefined");
Copy link
Member

Choose a reason for hiding this comment

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

use void 0 instead of undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I do that?

Copy link
Member

Choose a reason for hiding this comment

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

t.unaryExpression(t.numericalLiteral(0))

boopathi
boopathi reviewed May 9, 2017
View reviewed changes
packages/babel-plugin-minify-constant-folding/src/replacements.js Outdated
@@ -0,0 +1,123 @@
const other = Symbol("other");
Copy link
Member

Choose a reason for hiding this comment

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

rename to a more descriptive one ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does fallback sound better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@boopathi Ping?

Copy link
Member

Choose a reason for hiding this comment

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

Not really sure. What is this used for though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@boopathi It is called when the mapping object doesn’t provide a handler for the specified key. For example, the Array handler uses it to return the value at the specified index if the value can be coerced to an integer.

Copy link
Member

Choose a reason for hiding this comment

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

Just bikeshedding, maybe - NO_HANDLER_FOUND or FALLBACK_HANDLER ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@boopathi Fixed. Sorry for the delay.

@boopathi
Copy link
Member

boopathi commented Jun 8, 2017

can you rebase with master and fix the CI errors ?

@j-f1
Copy link
Contributor Author

j-f1 commented Jun 8, 2017

Rebased and CI passing, @boopathi.

@boopathi
Copy link
Member

boopathi commented Jun 8, 2017

Interesting why the smoke tests didn't kick in.

@j-f1
Copy link
Contributor Author

j-f1 commented Jun 11, 2017

@boopathi Do you want me to try and debug that?

boopathi
boopathi approved these changes Jun 13, 2017
View reviewed changes
Copy link
Member

@boopathi boopathi left a comment

Choose a reason for hiding this comment

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

LGTM.

@j-f1 good to merge?

@j-f1
Copy link
Contributor Author

j-f1 commented Jun 13, 2017

Sure! 👍

@boopathi boopathi merged commit 7e2b315 into babel:master Jun 13, 2017
@j-f1
Copy link
Contributor Author

j-f1 commented Jun 13, 2017

🎉 Thanks!

@merih merih mentioned this pull request Jun 15, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boopathi boopathi boopathi approved these changes

Assignees
No one assigned
Labels
Tag: New Feature Pull Request adding a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@j-f1 @boopathi