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

Starting lines with slashes #111

Closed
ausi opened this issue Apr 11, 2015 · 6 comments

Comments

@ausi
Copy link

commented Apr 11, 2015

The readme says starting a line with ( or [ is the only gotcha with omitting semicolons, but what about starting lines with / e.g. with a regular expression? The following code breaks:

var foo = function () {}
var bar = 'baz'

/baz/.test(bar) && foo(bar)

standard detects the error and writes Unexpected token ., but there are cases where standard isn’t able to detect such errors, e.g.:

var foo = function () {}
var bar = 'baz'

/ bar(')/.test(/' / +bar) && foo(bar)

standard doesn’t find an error but the code breaks with undefined is not a function.

Omitting semicolons would be nice, but how can I make sure I’m not falling into such ASI edge cases?

@mattdesl

This comment has been minimized.

Copy link

commented Apr 16, 2015

Personally I feel the following should be disallowed at the beginning of a line statement:

+ * / - ` , . 

They almost always appear in one-liners and clever shorthand (as in your example) that would be better suited inside an if statement.

Whereas ( and [ should produce errors unless preceded by a semicolon, for legitimate use cases like this: standard#80

Two eslint issues that seem to be holding some of these things back:
eslint/eslint#1983
eslint/eslint#746

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2015

Not sure if it's currently disallowed, but I sometimes like to do:

function (arg1, arg2, arg3) {
  return 'https://'
    + arg1
    + '/somethingverylongthatdoesntfitononeline'
    + arg2
    + '/'
    + arg3
}

alternatively it could be written as:

function (arg1, arg2, arg3) {
  let ret = 'https://'
  ret += arg1
  ret += '/somethingverylongthatdoesntfitononeline'
  ret += arg2
  ret += '/'
  ret += arg3
  return ret
}

I feel the second one is a bit clunkier, but don't feel strongly about it.

@mattdesl

This comment has been minimized.

Copy link

commented Apr 16, 2015

This is all about starting statements with ASI-breaking characters, so both of your examples should be fine.

The wording should be more explicit as pointed out here:
#110

@feross

This comment has been minimized.

Copy link
Member

commented Apr 21, 2015

While, it would be nice to throw an error on these less-common ASI-breaking cases, I don't want to add additional checks beyond what eslint supports. So, let's move the discussion there. Here's a thread where some of these ideas are being discussed: eslint/eslint#746.

Once eslint supports the rules we need, I'm happy to enable them to make this catch more of these edge cases. In the meantime, standard will just miss these cases. Sorry, no tool is perfect.

@feross feross closed this Apr 21, 2015

@dcousens

This comment has been minimized.

Copy link
Member

commented Apr 21, 2015

@feross I know this issue needs to be fixed upstream. But maybe we can keep the issue open until then? Closing this gives the impression its fixed IMHO.

@feross feross reopened this May 3, 2015

@feross

This comment has been minimized.

Copy link
Member

commented Jun 30, 2015

Okay, so there's been progress on this front! 👍

As @mattdesl originally suggested, the most troublesome operators + * / - are now forbidden at the beginning of a line, with the operator-linebreak rule. So (mostly as an accident) @ausi's second example is now caught by standard because the last line starts with a /.

var foo = function () {}
var bar = 'baz'

/ bar(')/.test(/' / + bar) && foo(bar)

I also enabled the new no-unexpected-multiline rule, which attempts to ensure that "two unrelated consecutive lines are not accidentally interpreted as a single expression". This rule also catches the above error.

If you find additional gotchas, please file an eslint issue.

I think this issue can finally be closed.

@feross feross closed this Jun 30, 2015

@lock lock bot locked as resolved and limited conversation to collaborators May 11, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
5 participants
You can’t perform that action at this time.