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

Remove duplicated semicolon #1784

Closed
wants to merge 1 commit into from
Closed

Remove duplicated semicolon #1784

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 30, 2018

It seems that in this file the very first ; isn't a must, because we've used a self-executed function. It should be enough.

It seems that in this file the very first `;` isn't a must, because
we've used a selfexecuted function. It should be enough.
@willin
Copy link
Contributor

willin commented Aug 31, 2018

i think it's useful.

i used to wrote this way years ago.

;(function (){
  'use strict';
  // xxx
})();

@ghost
Copy link
Author

ghost commented Aug 31, 2018

@willin:Is it a habit or some special thing?

Trott
Trott previously requested changes Aug 31, 2018
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

It's defensive coding. If a previous JS file does not end with a semicolon and this one is concatenated after it, then the start of this file could have unintended consequences. It's a good practice and should be left as is IMO. (We're not concatenating this file as far as I can tell, but we probably should consider concatenating files if we're not, and we should probably keep all our files concatenation-ready.)

@ghost
Copy link
Author

ghost commented Aug 31, 2018

@Trott:But we can avoid this from happening……And in fact, when I run npm run build and npm run serve. It seems everything is going well with me. So don't you think it's a little 'excessive defence'?

@Trott
Copy link
Member

Trott commented Aug 31, 2018

@Trott:But we can avoid this from happening……And in fact, when I run npm run build and npm run serve. It seems everything is going well with me. So don't you think it's a little 'excessive defence'?

The value of leaving the semicolon is small but non-zero while the value of removing it is zero.

The cost of leaving the semicolon is zero while the cost of removing it is small but non-zero.

Therefore, it should be left IMO.

@Trott Trott dismissed their stale review August 31, 2018 02:31

I don't actually feel that strongly about this

@willin
Copy link
Contributor

willin commented Aug 31, 2018

it's excessive.

many eslint rules are excessive. just a good habit.

@Trott
Copy link
Member

Trott commented Aug 31, 2018

I dismissed my review because I don't feel so strongly as to block this, but I think leaving the semicolon is preferable to removing it.

@Trott
Copy link
Member

Trott commented Aug 31, 2018

I can't believe we're having a variation on the "semicolon or no semicolon" argument. I need to find a new hobby. 😃(Yes, I know this is not the usual "semicolon or no semicolon" argument. It's just a joke.)

@AyushG3112
Copy link
Contributor

Can we add a semicolon at the end though?

@ghost
Copy link
Author

ghost commented Aug 31, 2018

@AyushG3112:This just depends because considering everyone has diff ideas on this, but according to real situation IMO:I don't think it needs here. Because we have codes controllable, and we cannot write codes that will miss ;.

See this:

<script src="/static/js/download.js" async defer></script>

@pierreneter
Copy link
Contributor

it's just a semicolon. let it there.

@pierreneter
Copy link
Contributor

@Maledong, read this post for more information: https://hackernoon.com/an-open-letter-to-javascript-leaders-regarding-no-semicolons-82cec422d67d

@ghost
Copy link
Author

ghost commented Aug 31, 2018

@pierreneter:Thanks but I cannot open your given link.
BTW:After looking at the codes and your deploy application, it seems that this file's js codes will be compressed and in order to make it clear, we have to add ; at the very beginning. So I closed this.

This pull request was closed.
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.

4 participants