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

2.0 question re ecmaVersion + feature flags #5249

Closed
ljharb opened this Issue Feb 13, 2016 · 19 comments

Comments

Projects
None yet
8 participants
@ljharb
Contributor

ljharb commented Feb 13, 2016

I have polyfills that require ecmaVersion 3, but in some of the code branches, use ES6 features. For example, https://github.com/es-shims/RegExp.prototype.flags/blob/master/.eslintrc#L6-L9.

http://eslint.org/docs/user-guide/migrating-to-2.0.0 implies that I can not use ecmaVersion 3, and also the regex U and Y flags. Is this correct?

If so, what is my recourse for using eslint v2 with this package?

@eslintbot eslintbot added the triage label Feb 13, 2016

@btmills

This comment has been minimized.

Show comment
Hide comment
@btmills

btmills Feb 13, 2016

Member

@ljharb can you provide some examples of code you're trying to parse? I read that as you're writing ES3-compatible code but using ES6 syntax, which doesn't make sense.

Member

btmills commented Feb 13, 2016

@ljharb can you provide some examples of code you're trying to parse? I read that as you're writing ES3-compatible code but using ES6 syntax, which doesn't make sense.

@btmills btmills added the needs info label Feb 13, 2016

@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot Feb 13, 2016

Hi @ljharb, thanks for the issue. It looks like there's not enough information for us to know how to help you. If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

If it's something else, please just provide as much additional information as possible. Thanks!

eslintbot commented Feb 13, 2016

Hi @ljharb, thanks for the issue. It looks like there's not enough information for us to know how to help you. If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

If it's something else, please just provide as much additional information as possible. Thanks!

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Feb 13, 2016

Contributor

@btmills https://github.com/es-shims/RegExp.prototype.flags/blob/master/test/index.js#L29-L33 for example - it makes perfect sense in any code that is doing feature detection, which this particular polyfill is.

Contributor

ljharb commented Feb 13, 2016

@btmills https://github.com/es-shims/RegExp.prototype.flags/blob/master/test/index.js#L29-L33 for example - it makes perfect sense in any code that is doing feature detection, which this particular polyfill is.

@btmills

This comment has been minimized.

Show comment
Hide comment
@btmills

btmills Feb 13, 2016

Member

It looks like the example there would be a runtime exception since you're using the constructor and not the equivalent literals. Since it's just function calls, it parses fine in ES3 mode - you only need the parser options if you're using literals.

Member

btmills commented Feb 13, 2016

It looks like the example there would be a runtime exception since you're using the constructor and not the equivalent literals. Since it's just function calls, it parses fine in ES3 mode - you only need the parser options if you're using literals.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Feb 13, 2016

Contributor

@btmills sorry I wasn't clear before - eslint v2 is currently warning on these lines

   31:18  error    Invalid flags supplied to RegExp constructor 'y'        no-invalid-regexp
   37:18  error    Invalid flags supplied to RegExp constructor 'u'        no-invalid-regexp

However, eslint v1 did not, because I have the feature flags enabled here

Contributor

ljharb commented Feb 13, 2016

@btmills sorry I wasn't clear before - eslint v2 is currently warning on these lines

   31:18  error    Invalid flags supplied to RegExp constructor 'y'        no-invalid-regexp
   37:18  error    Invalid flags supplied to RegExp constructor 'u'        no-invalid-regexp

However, eslint v1 did not, because I have the feature flags enabled here

@btmills

This comment has been minimized.

Show comment
Hide comment
@btmills

btmills Feb 14, 2016

Member

Ah, now I see what's going on. In the future, please be sure to include all of the requested information.

I'd recommend disabling the rule, either globally in your config or using inline comments. Since you have a shim, you've essentially supplanted the built-in RegExp constructor with something different, and no-invalid-regexp knows nothing about the replacement.

Member

btmills commented Feb 14, 2016

Ah, now I see what's going on. In the future, please be sure to include all of the requested information.

I'd recommend disabling the rule, either globally in your config or using inline comments. Since you have a shim, you've essentially supplanted the built-in RegExp constructor with something different, and no-invalid-regexp knows nothing about the replacement.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Feb 14, 2016

Contributor

I provided it initially by linking to the project in question, I apologize if it wasn't explicit enough.

I think this is a flaw in the assumptions made here - that the ecma features used are tightly coupled to the base ecma version set in the parserOptions. In v1, it was both possible and used to use a base ecma version that was lower than some of the ecma features required. In v2, it seems that it was decided that this valid and existing use case "didn't make sense".

I won't be adding a random rule-disabling comment, instead I'd be forced to never upgrade to eslint v2 because it decided to stop supporting my package's use case. That seems very unfortunate.

Contributor

ljharb commented Feb 14, 2016

I provided it initially by linking to the project in question, I apologize if it wasn't explicit enough.

I think this is a flaw in the assumptions made here - that the ecma features used are tightly coupled to the base ecma version set in the parserOptions. In v1, it was both possible and used to use a base ecma version that was lower than some of the ecma features required. In v2, it seems that it was decided that this valid and existing use case "didn't make sense".

I won't be adding a random rule-disabling comment, instead I'd be forced to never upgrade to eslint v2 because it decided to stop supporting my package's use case. That seems very unfortunate.

@btmills

This comment has been minimized.

Show comment
Hide comment
@btmills

btmills Feb 15, 2016

Member

@eslint/eslint-team What do you think of adding an option to no-invalid-regex to make it "shim-aware"? Such an option would tell the rule "I have a custom RegExp implementation that supports features x, y, and z." (Or in this case u and y probably.)

Member

btmills commented Feb 15, 2016

@eslint/eslint-team What do you think of adding an option to no-invalid-regex to make it "shim-aware"? Such an option would tell the rule "I have a custom RegExp implementation that supports features x, y, and z." (Or in this case u and y probably.)

@mysticatea

This comment has been minimized.

Show comment
Hide comment
@mysticatea

mysticatea Feb 15, 2016

Member

In my opinion, it's good to support the case: "I use polyfills but don't use transpilers."
I think it's one of popular options.

e.g. ESLint is using polyfills, but not using transpilers :D

Member

mysticatea commented Feb 15, 2016

In my opinion, it's good to support the case: "I use polyfills but don't use transpilers."
I think it's one of popular options.

e.g. ESLint is using polyfills, but not using transpilers :D

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Feb 15, 2016

Member

Problem I see with supporting polyfills is we will have to modify more rules then just this one. And it will be hard to configure them all properly.

Member

ilyavolodin commented Feb 15, 2016

Problem I see with supporting polyfills is we will have to modify more rules then just this one. And it will be hard to configure them all properly.

@btmills

This comment has been minimized.

Show comment
Hide comment
@btmills

btmills Feb 15, 2016

Member

That's my concern exactly. I think this would be a great case for a plugin containing a bunch of rules for shim-/polyfill-able cases.

Member

btmills commented Feb 15, 2016

That's my concern exactly. I think this would be a great case for a plugin containing a bunch of rules for shim-/polyfill-able cases.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Feb 15, 2016

Contributor

I think this was just an uninformed decision (to tie ecmaFeatures to the ecmaVersion). Rules which deal with syntax - which are unpolyfillable - absolutely make sense to tie to the ecmaVersion. However, any rules which do not, like the RegExp constructor, should not be.

Contributor

ljharb commented Feb 15, 2016

I think this was just an uninformed decision (to tie ecmaFeatures to the ecmaVersion). Rules which deal with syntax - which are unpolyfillable - absolutely make sense to tie to the ecmaVersion. However, any rules which do not, like the RegExp constructor, should not be.

@BYK

This comment has been minimized.

Show comment
Hide comment
@BYK

BYK Feb 15, 2016

Member

That's my concern exactly. I think this would be a great case for a plugin containing a bunch of rules for shim-/polyfill-able cases.

My thoughts exactly.

I think this was just an uninformed decision (to tie ecmaFeatures to the ecmaVersion). Rules which deal with syntax - which are unpolyfillable - absolutely make sense to tie to the ecmaVersion. However, any rules which do not, like the RegExp constructor, should not be.

I see where you are coming from that said AFAIK ECMAScript versions come with both built-in API specs (like what RegExp supports and what it doesn't etc.) in addition to syntax rules. @nzakas, @michaelficarra can you confirm me?

Member

BYK commented Feb 15, 2016

That's my concern exactly. I think this would be a great case for a plugin containing a bunch of rules for shim-/polyfill-able cases.

My thoughts exactly.

I think this was just an uninformed decision (to tie ecmaFeatures to the ecmaVersion). Rules which deal with syntax - which are unpolyfillable - absolutely make sense to tie to the ecmaVersion. However, any rules which do not, like the RegExp constructor, should not be.

I see where you are coming from that said AFAIK ECMAScript versions come with both built-in API specs (like what RegExp supports and what it doesn't etc.) in addition to syntax rules. @nzakas, @michaelficarra can you confirm me?

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Feb 15, 2016

Member

@ljharb The reason this change was implemented was to match other tools (acorn, escope, etc.) None of them had ability to fine-tune language features, and we had to create conversions for each one.

Member

ilyavolodin commented Feb 15, 2016

@ljharb The reason this change was implemented was to match other tools (acorn, escope, etc.) None of them had ability to fine-tune language features, and we had to create conversions for each one.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 15, 2016

Member

Let's not expand this case too far. The problem reported is isolated to this one rule, and the syntax in question is valid and parseable evennin ES3. Let's just add an option to the rule that allows loosening of this check. Maybe aalowConstructorFlags: ["u", "y"]?

Member

nzakas commented Feb 15, 2016

Let's not expand this case too far. The problem reported is isolated to this one rule, and the syntax in question is valid and parseable evennin ES3. Let's just add an option to the rule that allows loosening of this check. Maybe aalowConstructorFlags: ["u", "y"]?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Feb 15, 2016

Contributor

Thank you, that sounds like a totally legitimate and workable solution.

Contributor

ljharb commented Feb 15, 2016

Thank you, that sounds like a totally legitimate and workable solution.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 15, 2016

Member

Wow, didn't realize how badly autocomplete mangled that previous comment. Just to be clear, I think the solution here is to add allowConstructorFlags as an option that lets you pass an array of flags that should be valid regardless of the ecmaVersion setting.

Member

nzakas commented Feb 15, 2016

Wow, didn't realize how badly autocomplete mangled that previous comment. Just to be clear, I think the solution here is to add allowConstructorFlags as an option that lets you pass an array of flags that should be valid regardless of the ecmaVersion setting.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Feb 15, 2016

Contributor

+1, that seems better than the 1.x behavior even

Contributor

ljharb commented Feb 15, 2016

+1, that seems better than the 1.x behavior even

@afahim

This comment has been minimized.

Show comment
Hide comment
@afahim

afahim Feb 20, 2016

Contributor

I'll take a look at this over the weekend.

Contributor

afahim commented Feb 20, 2016

I'll take a look at this over the weekend.

afahim pushed a commit to afahim/eslint that referenced this issue Feb 21, 2016

afahim pushed a commit to afahim/eslint that referenced this issue Feb 21, 2016

afahim pushed a commit to afahim/eslint that referenced this issue Feb 21, 2016

afahim pushed a commit to afahim/eslint that referenced this issue Feb 21, 2016

afahim pushed a commit to afahim/eslint that referenced this issue Feb 21, 2016

ilyavolodin added a commit that referenced this issue Mar 1, 2016

Merge pull request #5352 from afahim/issue5249
Update: no-invalid-regexp allows custom flags (fixes #5249)

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

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