Fix: id-match mysteriously fails when defined in a comment. (fixes #9…

[aladdin-add]

…366)

What is the purpose of this pull request? (put an "X" next to item)

[x] Bug fix (template)

What changes did you make? (Give an overview)
fixes #9366

When the option value is a regex, we use levn.parse() and new Regex() to get it. It should be unescaped twice if working correctly:

levn.parse('\\\\n');   => '\\n'
new Regex('\\n');      => /\n/

But since levn don't support escape \, the parsed result would be '\\\\n' -- this is not right. this PR simply replace '\\\\' to '\\', so the result can be right.

Is there anything you'd like reviewers to focus on?

[not-an-aardvark]

Thanks for the PR!

Replying to #9366 (comment):

hmm..interesting! seems levn will replace "\'" to "\\'". (we are using it to parse comment config)

I think you were correct originally -- this seems like a bug in levn. For example, it's not possible use a newline in a string, because it gets parsed as \ followed by n:

JSON.parse(          '{ "foo": "\\n" }') // => { foo: '\n' }
levn.parse('Object', '{ "foo": "\\n" }') // => { foo: '\\n' }

To fix this issue, I think we need to handle all escape characters, not just \.

We could either:

• Fix the issue in levn (although this might be intended behavior)

• Replace all backslash escapes with the correct version ourselves. For example:

  const levnValue = levn.parse(string);
  
  // replace all strings with the correct backslash escapes
  return lodash.cloneDeepWith(levnValue, parsedValue => {
      if (typeof parsedValue === "string") {
          return replaceBackslashEscapes(parsedValue); // replaces \n with a newline, etc.
      }
  
      // clone the value normally
      return void 0;
  });

  The implementation of replaceBackslashEscapes could end up being a bit complicated, because levn has different escaping rules for single-quoted strings and double-quoted strings. ("\"" gets parsed as ", but '\"' gets parsed as \".) One option would be to use something like JSON.parse(`"${str}"`) to automatically replace \n with a newline, but we would need to handle the case where str contains double quotes (or escaped double quotes).

[aladdin-add]

thanks for quick response. I think I got your point, would it be better to specify an option to levn to do this -- so we no longer need to use lodash cloneDeepWith.

not sure it works, but worth a try, thoughts?

[not-an-aardvark]

This discussion

[not-an-aardvark]

I realize this is WIP, but this might be more readable if you use String.raw.

String.raw`\n` // => \n (backslash + newline)

[eslintbot]

LGTM

[aladdin-add]

@not-an-aardvark it can fix the issue. but has a side effect:

// `\` is optional, so either is fine. 
/* eslint id-match: [2, "^(([^$\\W]|\\$[a-f\\d]{2})+|[$_]\\w*|[^\\W\\d]\\w*|[A-Z]([A-Z_]*[A-Z])?)$"]*/
/* eslint id-match: [2, "^(([^$\W]|\$[a-f\d]{2})+|[$_]\w*|[^\W\d]\w*|[A-Z]([A-Z_]*[A-Z])?)$"]*/

[ilyavolodin]

@not-an-aardvark @aladdin-add What's the resolution on this PR? Is this something we want to fix, or should this be fixed in levn?

[aladdin-add]

When the option value is a regex, we use levn.parse() and new Regex() to get it. It should be unescaped twice if working correctly:

levn.parse('\\\\n');   => '\\n'
new Regex('\\n');      => /\n/

But since levn don't support escape \, the parsed result would be '\\\\n' -- this is not right. this PR simply replace '\\\\' to '\\', so the result can be right.

I'm not sure this is ideal. Hope we can figure out a better way. (can be discussed in TSC meeting?)