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 eslint-plugin-next node-attributes #36040

Closed
wants to merge 3 commits into from

Conversation

hyesungoh
Copy link

@hyesungoh hyesungoh commented Apr 9, 2022

Hello there. I'm always using it thankfully.

by the way I think the method below is highly readable

// before
return !!Object.keys(this.attributes).length

// after
return Boolean(Object.keys(this.attributes).length)

I wonder why you wrote like before code.

It may be that I'm not good enough to think that reason.

Thanks to read this pr and your service.

Feature

  • Telemetry added. In case of a feature if it's used or not.

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR, we don't have a lint rule specific to whether !! is used or Boolean so I think people tend to prefer the shorthand !!.

It seems there is a typo here as the .length and .hasValue checks need to be inside of the Boolean which may be a reason to continue using the shorthand as it's easier to avoid that kind of typo.

@hyesungoh hyesungoh requested a review from ijjk April 9, 2022 22:24
@hyesungoh
Copy link
Author

Hi, thanks for the PR, we don't have a lint rule specific to whether !! is used or Boolean so I think people tend to prefer the shorthand !!.

It seems there is a typo here as the .length and .hasValue checks need to be inside of the Boolean which may be a reason to continue using the shorthand as it's easier to avoid that kind of typo.

I changed you said 😄
Thanks to answer me ! I learned alot.

@ijjk
Copy link
Member

ijjk commented Apr 9, 2022

Stats from current PR

Default Build (Decrease detected ✓)
General
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
buildDuration 15s 15s ⚠️ +71ms
buildDurationCached 6s 6.1s ⚠️ +24ms
nodeModulesSize 478 MB 478 MB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
/ failed reqs 0 0
/ total time (seconds) 3.004 3.048 ⚠️ +0.04
/ avg req/sec 832.16 820.32 ⚠️ -11.84
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.161 1.168 ⚠️ +0.01
/error-in-render avg req/sec 2153.48 2139.62 ⚠️ -13.86
Client Bundles (main, webpack)
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
925.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42 kB 42 kB
main-HASH.js gzip 28 kB 28 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.7 kB 71.7 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
_app-HASH.js gzip 1.36 kB 1.36 kB
_error-HASH.js gzip 192 B 192 B
amp-HASH.js gzip 309 B 309 B
css-HASH.js gzip 327 B 327 B
dynamic-HASH.js gzip 3.05 kB 3.05 kB
head-HASH.js gzip 351 B 351 B
hooks-HASH.js gzip 920 B 920 B
image-HASH.js gzip 5.68 kB 5.68 kB
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 2.32 kB 2.32 kB
routerDirect..HASH.js gzip 320 B 320 B
script-HASH.js gzip 387 B 387 B
withRouter-HASH.js gzip 319 B 319 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 15.9 kB 15.9 kB
Client Build Manifests
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
_buildManifest.js gzip 460 B 460 B
Overall change 460 B 460 B
Rendered Page Sizes
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
index.html gzip 531 B 531 B
link.html gzip 544 B 544 B
withRouter.html gzip 524 B 524 B
Overall change 1.6 kB 1.6 kB

Default Build with SWC
General
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
buildDuration 18s 17.8s -136ms
buildDurationCached 6s 6s ⚠️ +38ms
nodeModulesSize 478 MB 478 MB
Page Load Tests Overall increase ✓
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
/ failed reqs 0 0
/ total time (seconds) 2.99 3.025 ⚠️ +0.03
/ avg req/sec 836 826.52 ⚠️ -9.48
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.18 1.173 -0.01
/error-in-render avg req/sec 2118.05 2130.94 +12.89
Client Bundles (main, webpack)
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
925.HASH.js gzip 178 B 178 B
framework-HASH.js gzip 42.3 kB 42.3 kB
main-HASH.js gzip 28.3 kB 28.3 kB
webpack-HASH.js gzip 1.45 kB 1.45 kB
Overall change 72.2 kB 72.2 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
_app-HASH.js gzip 1.35 kB 1.35 kB
_error-HASH.js gzip 179 B 179 B
amp-HASH.js gzip 313 B 313 B
css-HASH.js gzip 325 B 325 B
dynamic-HASH.js gzip 3.03 kB 3.03 kB
head-HASH.js gzip 351 B 351 B
hooks-HASH.js gzip 921 B 921 B
image-HASH.js gzip 5.74 kB 5.74 kB
index-HASH.js gzip 261 B 261 B
link-HASH.js gzip 2.38 kB 2.38 kB
routerDirect..HASH.js gzip 322 B 322 B
script-HASH.js gzip 388 B 388 B
withRouter-HASH.js gzip 317 B 317 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 16 kB 16 kB
Client Build Manifests
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
_buildManifest.js gzip 459 B 459 B
Overall change 459 B 459 B
Rendered Page Sizes
vercel/next.js canary hyesungoh/next.js refactor/node-attributes Change
index.html gzip 531 B 531 B
link.html gzip 544 B 544 B
withRouter.html gzip 527 B 527 B
Overall change 1.6 kB 1.6 kB
Commit: 7a7097a

@balazsorban44
Copy link
Member

Thanks for the PR, but I don't think this adds any value. The code snippets have the same effect.

@hyesungoh hyesungoh deleted the refactor/node-attributes branch May 4, 2022 05:00
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2022
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.

3 participants