workaround for broken Edge versions 15-18 #304

ekilah · 2020-08-17T20:08:40Z

Fixes #303

I have run npm run lint:fix to ensure Prettier and ESLint have passed
Coveralls bot has replied that the tests pass with 100% code coverage (npm test)
I have updated CHANGELOG.md (and API.md if this is an API ch

Summary of problem

We saw these reports in our bug tracker:

Error unsupported regexp flag "n": /[0-9]/undefined 
    node_modules/parsimmon/build/parsimmon.umd.min.js:1:6716 Anonymous function
    node_modules/parsimmon/build/parsimmon.umd.min.js:1:6560 K
    node_modules/parsimmon/build/parsimmon.umd.min.js:1:11055 Anonymous function
    node_modules/parsimmon/build/parsimmon.umd.min.js:1:348 r
    node_modules/parsimmon/build/parsimmon.umd.min.js:1:407 Anonymous function
    node_modules/parsimmon/build/parsimmon.umd.min.js:1:199 Anonymous function
    node_modules/parsimmon/build/parsimmon.umd.min.js:1 Anonymous function
    webpack/bootstrap:25:3 __webpack_require__
    https://mywebsite.com/static/js/main.abc.js:1:123 Anonymous function
    webpack/bootstrap:25:3 __webpack_require__

These errors were coming from MS Edge versions 15-18, but I couldn't find a way to download and install that version to test for myself. It seemed like that browser was rendering the literal string "undefined" at the end of regular expressions when they were printed as strings.

Research

To get some data from our prod environment, I added the following code to parsimmon (you can see the fork I used here: master...ekilah:debuggingEdge18):

function flags(re) {
  var s = "" + re;
  var fs = s.slice(s.lastIndexOf("/") + 1);
  if (fs === "undefined" || fs === undefined) {
    // eslint-disable-next-line no-console, no-undef
    console.warn('Flags of regex were undefined.', {re: re, s: s, fs: fs, typeofFs: typeof fs})
    return ""
  }
  return fs
}

and got this output from Edge versions 17 and 18:

{
  "re":{},
  "s":"/[0-9]/undefined",
  "fs":"undefined",
  "typeofFs":"string"
}

I also added this above var digit = regexp(/[0-9]/).desc("a digit"); so it would run when parsimmon is imported in my project:

var reTest = new RegExp(/[0-9]/)
var reWithFlagTest = new RegExp(/[0-9]/i)
var strTest = new RegExp("[0-9]")

// eslint-disable-next-line no-console, no-undef
console.warn('Debugging regexes:', {
  re: {
    asString: '' + reTest,
    flagsDefined: typeof reTest.flags === 'string',
    flags: reTest.flags,
  },
  reWithFlag: {
    asString: '' + reWithFlagTest,
    flagsDefined: typeof reWithFlagTest.flags === 'string',
    flags: reWithFlagTest.flags,
  },
  str: {
    asString: '' + strTest,
    flagsDefined: typeof strTest.flags === 'string',
    flags: strTest.flags,
  },
})

and got this output from Edge versions 17 and 18:

{
  "re":{
    "asString":"/[0-9]/undefined",
    "flagsDefined":false
  },
  "reWithFlag":{
    "asString":"/[0-9]/undefined",
    "flagsDefined":false
  },
  "str":{
    "asString":"/[0-9]/undefined",
    "flagsDefined":false
  }
}

Fix

To fix this, we can detect undefined as the flags string and overwrite it.

wavebeem · 2020-08-17T20:48:43Z

Thanks for the bugfix! I appreciate how tricky this is to track down.

I'd prefer if we feature detected .flags on the regexp and then constructed
our own flags from scratch. This way we won't ignore regexp flags in Edge. Plus,
I think the code is easier to follow this way.

function flags(re) {
  if (re.flags !== undefined) {
    return re.flags;
  }
  return [
    re.global ? "g" : "",
    re.ignoreCase ? "i" : "",
    re.multiline ? "m" : "",
    re.unicode ? "u" : "",
    re.sticky ? "y" : "",
  ].join("");
}

These boolean properties are supported all the way back to IE 5.5 apparently, so
it should be safe to use those.

wavebeem · 2020-08-17T22:26:34Z

Thanks! Looks like you'll need to run prettier npm run lint:fix

After that, you'll need to make the test coverage go back up to 100%. I would suggested modifying a regexp like:

var oldRegExp = /a/gimuy;
// Simulate old RegExp without flags property
Object.defineProperty(oldRegExp, "flags", { value: undefined });

ekilah · 2020-08-18T16:51:25Z

Sure, I added that instead. I agree the way flags worked before was a bit messy, and I wasn't aware of these other flags before.

I tested the 5 boolean flags in my production environment, it looks like re.unicode doesn't work properly on these versions of Edge:

Using this regex: /[0-9]/gimuy I logged this:

    {
        regexStr: '' + regex,
        global: regex.global ?? 'undefined var',
        ignoreCase: regex.ignoreCase ?? 'undefined var',
        multiline: regex.multiline ?? 'undefined var',
        unicode: regex.unicode ?? 'undefined var',
        sticky: regex.sticky ?? 'undefined var',
      }

and got this:

Mostly just noting this for you if it ever comes up. I doubt it will!

As for the tests and whatnot, sure, I can do some of that maybe later in the week. As a suggestion - the README on contributing should mention the linter and/or test coverage expectations. I'd add them but I don't know what the expectations are 😄

wavebeem · 2020-08-18T17:38:01Z

Mostly just noting this for you if it ever comes up. I doubt it will!

Good to know, thanks! Old Edge still had some warts for sure.

As for the tests and whatnot, sure, I can do some of that maybe later in the week. As a suggestion - the README on contributing should mention the linter and/or test coverage expectations. I'd add them but I don't know what the expectations are 😄

My bad, I swear I had those covered. I'll add a pull request template with a checklist/info

wavebeem · 2020-08-18T17:42:36Z

https://github.com/jneen/parsimmon/blob/master/PULL_REQUEST_TEMPLATE.md

I can edit this if I forgot anything 😄

coveralls · 2020-08-25T00:43:55Z

Coverage decreased (-0.6%) to 99.438% when pulling 37b45b1 on ekilah:fixEdge18 into cb77d09 on jneen:master.

coveralls · 2020-08-25T00:43:55Z

Coverage remained the same at 100.0% when pulling 62246d8 on ekilah:fixEdge18 into cb77d09 on jneen:master.

ekilah · 2020-08-25T01:13:52Z

test/.eslintrc

@@ -7,5 +7,8 @@
    "Parsimmon": true,
    "assert": true,
    "testSetScenario": true
+  },
+  "rules": {
+    "no-invalid-regexp": ["error", {"allowConstructorFlags": ["u", "y"]}]


ESLint doesn't like these flags by default. And this override doesn't work if you define the Regexp like /a/gimuy, you have to use the new Regexp constructor, apparently.

ekilah · 2020-08-25T01:15:45Z

@wavebeem alright, I think this should be complete. I added the changelog entries, but I guess it's a bit weird since idk if you'll actually release it with that version number. Let me know if this needs to be squashed or if you'll do that, etc.

wavebeem

This looks great all around, but I don't think Parsimmon.flags needs to be exported. I don't think that function will be useful to anyone using Parsimmon.

In order to test this you could do something like:

assert.throws(function() {
  Parsimmon.regexp(/a/g);
});
assert.throws(function() {
  Parsimmon.regexp(/a/y);
});
assert.doesNotThrow(function() {
  Parsimmon.regexp(/a/i);
});
assert.doesNotThrow(function() {
  Parsimmon.regexp(/a/m);
});
assert.doesNotThrow(function() {
  Parsimmon.regexp(/a/u);
});
// ...and then test the "messed up" regexps with flags set to undefined

ekilah · 2020-08-25T21:43:51Z

Personally I think the test is harder to read / isn't as obviously testing the behavior it is testing this way, and it doesn't test the return value of flags() anymore.

wavebeem

I feel you on the tests being harder to read now, but I don't want to needlessly expand Parsimmonn's already large API.

wavebeem · 2020-08-25T21:48:20Z

CHANGELOG.md

@@ -1,7 +1,6 @@
 ## version 1.16.0 (2020-08-25)

- Adds `Parsimmon.flags(regexp)` export
- Fixes `Parsimmon.flags(regexp)` to work in older browsers (Edge versions 15-18)
+- Fixes `flags(regexp)` to work in older browsers (Edge versions 15-18), which fixes crashes on startup in those old clients


I think you could shorten this to Fixes a crash in MS Edge 15-18 when using Parsimmon.regexp

Also feel free to add your github handle to the changelog here as credit if you'd like 😄

sure, solved, and thanks!

wavebeem · 2020-08-25T22:15:12Z

Looks great! Thanks for your work on this. I'll merge this and release it in a few hours or so.

wavebeem · 2020-08-26T00:25:11Z

@ekilah 1.16.0 is up! https://www.npmjs.com/package/parsimmon

ekilah · 2020-08-26T01:58:13Z

Awesome 🥳 Thanks for your help debugging & getting this live!

workaround for broken Edge versions 15-18

58666bc

ekilah mentioned this pull request Aug 17, 2020

MS Edge 18 error #303

Closed

use .flags when it is present

cc668ff

ekilah added 2 commits August 24, 2020 17:31

lint:fix

9372b12

add test for flags function

37b45b1

ekilah added 2 commits August 24, 2020 18:08

100% test coverage

0577c93

markdown files

a0408b4

ekilah commented Aug 25, 2020

View reviewed changes

remove eslint-ignore

d07bcb7

wavebeem requested changes Aug 25, 2020

View reviewed changes

dont export flags for test

1033d7b

wavebeem approved these changes Aug 25, 2020

View reviewed changes

changelog

62246d8

wavebeem merged commit 8d3dfd2 into jneen:master Aug 26, 2020

ekilah deleted the fixEdge18 branch August 26, 2020 01:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

workaround for broken Edge versions 15-18 #304

workaround for broken Edge versions 15-18 #304

ekilah commented Aug 17, 2020 •

edited

Loading

wavebeem commented Aug 17, 2020

wavebeem commented Aug 17, 2020

ekilah commented Aug 18, 2020

wavebeem commented Aug 18, 2020

wavebeem commented Aug 18, 2020

coveralls commented Aug 25, 2020

coveralls commented Aug 25, 2020 •

edited

Loading

ekilah Aug 25, 2020 •

edited

Loading

ekilah commented Aug 25, 2020 •

edited

Loading

wavebeem left a comment

ekilah commented Aug 25, 2020

wavebeem left a comment

wavebeem Aug 25, 2020

ekilah Aug 25, 2020

wavebeem commented Aug 25, 2020

wavebeem commented Aug 26, 2020

ekilah commented Aug 26, 2020

workaround for broken Edge versions 15-18 #304

workaround for broken Edge versions 15-18 #304

Conversation

ekilah commented Aug 17, 2020 • edited Loading

Summary of problem

Research

Fix

wavebeem commented Aug 17, 2020

wavebeem commented Aug 17, 2020

ekilah commented Aug 18, 2020

wavebeem commented Aug 18, 2020

wavebeem commented Aug 18, 2020

coveralls commented Aug 25, 2020

coveralls commented Aug 25, 2020 • edited Loading

ekilah Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

ekilah commented Aug 25, 2020 • edited Loading

wavebeem left a comment

Choose a reason for hiding this comment

ekilah commented Aug 25, 2020

wavebeem left a comment

Choose a reason for hiding this comment

wavebeem Aug 25, 2020

Choose a reason for hiding this comment

ekilah Aug 25, 2020

Choose a reason for hiding this comment

wavebeem commented Aug 25, 2020

wavebeem commented Aug 26, 2020

ekilah commented Aug 26, 2020

ekilah commented Aug 17, 2020 •

edited

Loading

coveralls commented Aug 25, 2020 •

edited

Loading

ekilah Aug 25, 2020 •

edited

Loading

ekilah commented Aug 25, 2020 •

edited

Loading