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

87th Line on express.js starts with ; #3677

Closed
thesageinpilani opened this Issue Jun 25, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@thesageinpilani

thesageinpilani commented Jun 25, 2018

express is a great framework. I was going through the code and i found that 87th line starts with ';', i am very new to both nodejs and expressjs and i may be completely wrong. does the ; in line 87, is actually the semi colon for line 81? or the line 87 intentionally start with a semi colon?. I am very new to express JS and if it is intentional, can you kindly please explain the purpose of that starting ;, Thank you😀

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jun 25, 2018

Member

Hi! Thanks for taking a look. This is just following the StandardJS style and is intentional: https://standardjs.com/rules-en.html#semicolons

Also see previous issue #3558 as well :)

Member

dougwilson commented Jun 25, 2018

Hi! Thanks for taking a look. This is just following the StandardJS style and is intentional: https://standardjs.com/rules-en.html#semicolons

Also see previous issue #3558 as well :)

@LinusU

This comment has been minimized.

Show comment
Hide comment
@LinusU

LinusU Jun 25, 2018

Member

Could potentially be nice to extract the list into a variable, or even unroll the loop so that it doesn't happen when loading the module.

edit: unrolling the loop increases the performance of that part by ~33%, not sure it's worth the increased code size though... Naming the list would probably be nice though: var NO_LONGER_BUNDLED = [...], NO_LONGER_BUNDLED.forEach(...)

Member

LinusU commented Jun 25, 2018

Could potentially be nice to extract the list into a variable, or even unroll the loop so that it doesn't happen when loading the module.

edit: unrolling the loop increases the performance of that part by ~33%, not sure it's worth the increased code size though... Naming the list would probably be nice though: var NO_LONGER_BUNDLED = [...], NO_LONGER_BUNDLED.forEach(...)

@wesleytodd

This comment has been minimized.

Show comment
Hide comment
@wesleytodd

wesleytodd Jun 25, 2018

Member

@LinusU Since this is at startup I am not sure a perf change is worth the churn. Also FWIW, I think all of this is going to be removed in 5 since it has been there for all of 4.

Member

wesleytodd commented Jun 25, 2018

@LinusU Since this is at startup I am not sure a perf change is worth the churn. Also FWIW, I think all of this is going to be removed in 5 since it has been there for all of 4.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jun 26, 2018

Member

Yea, the block is deleted in 5.0 branch (AFAIK), but I think we probably should at least move it into a var as @LinusU proposed in the 4.x series. The reasons I'm thinking that is as follows: (1) there has already been two questions (at least) about the "strange placement" of the semicolon so far and (2) it really is just trying to be too-clever and thus clashing with no semicolons style, which is why it seems weird (and an intermediate variable would fix that).

If you guys agree, one of you (@LinusU ?) please feel welcome to open a PR moving it to a variable 👍

Member

dougwilson commented Jun 26, 2018

Yea, the block is deleted in 5.0 branch (AFAIK), but I think we probably should at least move it into a var as @LinusU proposed in the 4.x series. The reasons I'm thinking that is as follows: (1) there has already been two questions (at least) about the "strange placement" of the semicolon so far and (2) it really is just trying to be too-clever and thus clashing with no semicolons style, which is why it seems weird (and an intermediate variable would fix that).

If you guys agree, one of you (@LinusU ?) please feel welcome to open a PR moving it to a variable 👍

LinusU added a commit to LinusU/express that referenced this issue Jun 27, 2018

@LinusU

This comment has been minimized.

Show comment
Hide comment
@LinusU

LinusU Jun 27, 2018

Member

👉 #3678

Member

LinusU commented Jun 27, 2018

👉 #3678

dougwilson added a commit to LinusU/express that referenced this issue Sep 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment