Repository: jneen/parsimmon
Pull request #304 (2020-08-17 20:08:40)

workaround for broken Edge versions 15-18
Fixes https://github.com/jneen/parsimmon/issues/303

- [x] I have run `npm run lint:fix` to ensure Prettier and ESLint have passed
- [x] Coveralls bot has replied that the tests pass with 100% code coverage (`npm test`)
- [x] 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: https://github.com/jneen/parsimmon/compare/master...ekilah:debuggingEdge18):

```js

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:


```json
{
  "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:

```js
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:

```json
{
  "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: 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.

```js
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: 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:

```js
var oldRegExp = /a/gimuy;
// Simulate old RegExp without flags property
Object.defineProperty(oldRegExp, "flags", { value: undefined });
```
ekilah: 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:

![image](https://user-images.githubusercontent.com/3213339/90541748-edf61600-e137-11ea-89c1-ce55a99bfa6a.png)


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: > 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

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7efafbe..7569d1c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,7 @@
+## version 1.16.0 (2020-08-25)
+
+- Fixes a crash in MS Edge 15-18 when using `Parsimmon.regexp` (thanks @ekilah)
+
 ## version 1.15.0 (2020-07-27)
 
 - Adds support for the `s` (`dotAll`) flag in `Parsimmon.regexp`
diff --git a/PULL_REQUEST_TEMPLATE.md b/PULL_REQUEST_TEMPLATE.md
new file mode 100644
index 0000000..a19b256
--- /dev/null
+++ b/PULL_REQUEST_TEMPLATE.md
@@ -0,0 +1,3 @@
+- [ ] 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 change)
diff --git a/src/parsimmon.js b/src/parsimmon.js
index 028f701..3a19356 100644
--- a/src/parsimmon.js
+++ b/src/parsimmon.js
@@ -694,8 +694,17 @@ function formatError(input, error) {
 }
 
 function flags(re) {
-  var s = "" + re;
-  return s.slice(s.lastIndexOf("/") + 1);
+  if (re.flags !== undefined) {
+    return re.flags;
+  }
+  // legacy browser support
+  return [
+    re.global ? "g" : "",
+    re.ignoreCase ? "i" : "",
+    re.multiline ? "m" : "",
+    re.unicode ? "u" : "",
+    re.sticky ? "y" : ""
+  ].join("");
 }
 
 function anchoredRegexp(re) {


Test patch:
diff --git a/test/.eslintrc b/test/.eslintrc
index 4c7229e..cc00932 100644
--- a/test/.eslintrc
+++ b/test/.eslintrc
@@ -7,5 +7,8 @@
     "Parsimmon": true,
     "assert": true,
     "testSetScenario": true
+  },
+  "rules": {
+    "no-invalid-regexp": ["error", {"allowConstructorFlags": ["u", "y"]}]
   }
 }
diff --git a/test/core/flags.test.js b/test/core/flags.test.js
new file mode 100644
index 0000000..ebdc7e5
--- /dev/null
+++ b/test/core/flags.test.js
@@ -0,0 +1,72 @@
+"use strict";
+
+testSetScenario(function() {
+  describe("flags()", function() {
+    it("works in modern browsers", function() {
+      assert.throws(function() {
+        Parsimmon.regexp(new RegExp("a", "g"));
+      });
+      assert.doesNotThrow(function() {
+        Parsimmon.regexp(new RegExp("a", "i"));
+      });
+      assert.doesNotThrow(function() {
+        Parsimmon.regexp(new RegExp("a", "m"));
+      });
+      assert.doesNotThrow(function() {
+        Parsimmon.regexp(new RegExp("a", "u"));
+      });
+      assert.throws(function() {
+        Parsimmon.regexp(new RegExp("a", "y"));
+      });
+    });
+
+    it("works on legacy browsers without Regexp.flags property with flags", function() {
+      var oldRegExpG = new RegExp("a", "g");
+      var oldRegExpI = new RegExp("a", "i");
+      var oldRegExpM = new RegExp("a", "m");
+      var oldRegExpU = new RegExp("a", "u");
+      var oldRegExpY = new RegExp("a", "y");
+      var oldRegExps = [
+        oldRegExpG,
+        oldRegExpI,
+        oldRegExpM,
+        oldRegExpU,
+        oldRegExpY
+      ];
+
+      // Simulate old RegExp without the flags property
+      oldRegExps.map(function(r) {
+        Object.defineProperty(r, "flags", { value: undefined });
+        assert.strictEqual(r.flags, undefined);
+      });
+
+      assert.throws(function() {
+        Parsimmon.regexp(oldRegExpG);
+      });
+      assert.doesNotThrow(function() {
+        Parsimmon.regexp(oldRegExpI);
+      });
+      assert.doesNotThrow(function() {
+        Parsimmon.regexp(oldRegExpM);
+      });
+      assert.doesNotThrow(function() {
+        Parsimmon.regexp(oldRegExpU);
+      });
+      assert.throws(function() {
+        Parsimmon.regexp(oldRegExpY);
+      });
+    });
+
+    it("works on legacy browsers without Regexp.flags property without flags", function() {
+      var oldRegExp = new RegExp("a", "");
+
+      // Simulate old RegExp without the flags property
+      Object.defineProperty(oldRegExp, "flags", { value: undefined });
+      assert.strictEqual(oldRegExp.flags, undefined);
+
+      assert.doesNotThrow(function() {
+        Parsimmon.regexp(oldRegExp);
+      });
+    });
+  });
+});