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

refactor: small readability improvements #5748

Merged
merged 3 commits into from
May 16, 2019
Merged

refactor: small readability improvements #5748

merged 3 commits into from
May 16, 2019

Conversation

manniL
Copy link
Member

@manniL manniL commented May 15, 2019

Changes

  • Refactor !!sth to Boolean(sth)
  • Refactor { abc: abc } to { abc }
  • Refactor const abc = someArray[0] to const [abc] = someArray

@manniL manniL changed the title Refactor/readability refactor: improve code base readability and consistency May 15, 2019
@@ -128,7 +128,7 @@ function mapTransitions(Components, to, from) {

async function loadAsyncComponents(to, from, next) {
// Check if route path changed (this._pathChanged), only if the page is not an error (for validate())
this._pathChanged = !!app.nuxt.err || from.path !== to.path
this._pathChanged = Boolean(app.nuxt.err) || from.path !== to.path
Copy link
Member

Choose a reason for hiding this comment

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

neat: If we swipe conditions, we can prevent unnecessary Boolean cast where from.path !== to.path

Suggested change
this._pathChanged = Boolean(app.nuxt.err) || from.path !== to.path
this._pathChanged = from.path !== to.path || Boolean(app.nuxt.err)

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean this? ☺️

Suggested change
this._pathChanged = Boolean(app.nuxt.err) || from.path !== to.path
this._pathChanged = from.path !== to.path || app.nuxt.err

Copy link
Member

@pi0 pi0 May 15, 2019

Choose a reason for hiding this comment

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

Well actually it depends. || app.nuxt.err can cause _pathChanged to be instance of err instead of Boolean (true/false). This is why we use !! or Boolean. If such type safety is not important we can omit Boolean everywhere :)

Copy link
Member

@clarkdo clarkdo left a comment

Choose a reason for hiding this comment

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

@manniL Good changes.

For Boolean, I agree with using it in property assignment this.foo = Boolean(bar) || ....

For local variable or if statement, !! may have some benefits.

if( !!this.app.err && a !== 'abc'){
    console.log(1)
}

// after terser compression, !! will be removed, Boolean(..) won't

this.app.err&&"abc"!==a&&console.log(1)

The !! is removed, the code size is smaller and there is no reference leak risk in these cases, so it should be fine.

@manniL
Copy link
Member Author

manniL commented May 15, 2019

@clarkdo Didn't know about the difference between !! and Boolean for terser minification/compression 😲

The question is if it's worth the few bytes, but the trade-off is always present 🙈

@pi0
Copy link
Member

pi0 commented May 15, 2019

Terser is only effective on vue-app files. BTW agree to keep consistency and byte-saving where we can. The benefit of Boolean is type safety and readability.

@manniL manniL marked this pull request as ready for review May 15, 2019 17:14
@manniL manniL changed the title refactor: improve code base readability and consistency refactor: small readability improvements May 15, 2019
@manniL
Copy link
Member Author

manniL commented May 15, 2019

I'd keep that PR as is and create new ones for larger changes ☺️

@manniL
Copy link
Member Author

manniL commented May 15, 2019

@pi0 IMO no, I also use const { color } = myFavorites instead of const color = myFavorites.color when it comes to objects :D

But if you disagree with that I'm happy to revert ☺️

@codecov-io
Copy link

codecov-io commented May 15, 2019

Codecov Report

Merging #5748 into dev will increase coverage by 0.03%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             dev    #5748      +/-   ##
=========================================
+ Coverage   95.6%   95.64%   +0.03%     
=========================================
  Files         82       82              
  Lines       2662     2662              
  Branches     683      683              
=========================================
+ Hits        2545     2546       +1     
+ Misses        98       97       -1     
  Partials      19       19
Impacted Files Coverage Δ
packages/builder/src/context/template.js 100% <ø> (ø) ⬆️
packages/webpack/src/config/base.js 94.94% <ø> (ø) ⬆️
packages/cli/src/commands/build.js 86.36% <0%> (ø) ⬆️
packages/webpack/src/plugins/vue/client.js 95% <100%> (ø) ⬆️
packages/babel-preset-app/src/index.js 96.77% <100%> (ø) ⬆️
packages/vue-renderer/src/renderer.js 94.3% <0%> (+0.81%) ⬆️

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 c2c2945...c9363d9. Read the comment docs.

@pi0
Copy link
Member

pi0 commented May 15, 2019

@manniL No both array and obj destruction are nice things. I just meant about single value arrays. We can do it for consistency or keep [0] for more readability. I'm OK with both.

@pi0 pi0 requested a review from clarkdo May 16, 2019 08:27
Copy link
Contributor

@kevinmarrec kevinmarrec left a comment

Choose a reason for hiding this comment

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

Good changes @manniL !

Regarding single value arrays access, I've exact same POV as @pi0 and I'm also OK with both 👍

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

Successfully merging this pull request may close these issues.

None yet

6 participants