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.

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

chore: reducing duplicate if into single statement #22

Closed
wants to merge 1 commit into from

Conversation

Pruxis
Copy link
Collaborator

@Pruxis Pruxis commented Feb 28, 2019

As I was reading through the code I noticed this, a minor improvement.

As I was reading through the code I noticed this, a minor improvement.
@Pruxis
Copy link
Collaborator Author

Pruxis commented Feb 28, 2019

I noticed that this is done on a lot of components, I'll add the rest to this pull request when I find more time.

@codecov-io
Copy link

codecov-io commented Feb 28, 2019

Codecov Report

Merging #22 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #22   +/-   ##
======================================
  Coverage    94.6%   94.6%           
======================================
  Files          17      17           
  Lines         278     278           
  Branches       48      47    -1     
======================================
  Hits          263     263           
  Misses         15      15
Impacted Files Coverage Δ
src/Error.tsx 90% <100%> (ø) ⬆️

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 68f0926...b133d43. Read the comment docs.

@JoviDeCroock
Copy link
Owner

JoviDeCroock commented Feb 28, 2019

Some uglifiers don't remove this code in production if you don't make it a dev check, so when adding the && !component it could be left untouched. I'll test this when I can.

Judging from the bundlesize output, terser is handling this correctly.

Note that this was last time I checked.

To come back to the "it seems to happen in a lot of components", it will be in the three hooks and the three components probably.

UPDATE: this seems resolved, probably was an issue with the old webpack uglifier, so seems a really good PR. Thanks again for the great work!

Repository owner deleted a comment from allcontributors bot Feb 28, 2019
@allcontributors
Copy link
Contributor

@JoviDeCroock

I've put up a pull request to add @Pruxis! 🎉

@Pruxis
Copy link
Collaborator Author

Pruxis commented Feb 28, 2019

Hmhm, I haven't tested this on older uglifiers but have never had issues with Terser & babel-uglify that's default in CRA at this point. I don't have a lot of free time to test this thoroughly tho

@JoviDeCroock
Copy link
Owner

JoviDeCroock commented Feb 28, 2019

No worries, I'll take the testing on me. Just saying but avoid using babel-minify this can cause some nasty performance downgrades in React.

Also this PR has no expiry date, not a crucial feature or anything just fun to give people quality.

@JoviDeCroock
Copy link
Owner

Thanks for helping out with this, I've implemented it myself but credits still go to you for noticing it! Thanks again for the wonderfull PR's

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

Successfully merging this pull request may close these issues.

3 participants