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

maybe rewrite three regexps for XS compatibility #4

Open
warner opened this issue Jul 2, 2020 · 6 comments
Open

maybe rewrite three regexps for XS compatibility #4

warner opened this issue Jul 2, 2020 · 6 comments

Comments

@warner
Copy link

warner commented Jul 2, 2020

We noticed three regexps in babel-standalone which are rejected by the XS regexp parser. These showed up as test failures in the agoric-sdk XS integration CI run. That test uses XS to compile a program that incorporates the tests from two of our agoric-sdk modules, and those test files recently acquired a dependency upon SES (in Agoric/agoric-sdk#1244), and SES uses babel-standalone. The first time XS was exposed to babel-standalone, we got an error like this:

https://github.com/Agoric/agoric-sdk/runs/824570388?check_suite_focus=true#step:10:75

The regexps it rejected were:

/home/runner/work/agoric-sdk/agoric-sdk/node_modules/@agoric/babel-standalone/babel.js:21342: error: ^\)] invalid character!

line 21341:

  function parseSourceMapInput(str) {
    return JSON.parse(str.replace(/^\)]}'[^\n]*\n/, ''));
  }
/home/runner/work/agoric-sdk/agoric-sdk/node_modules/@agoric/babel-standalone/babel.js:66549: error: ^(?:] invalid character!

line 66548:

        } else if (!hasUnicodeFlag && (res = matchReg(/^(?:]|})/))) {
          return createCharacter(res);
/home/runner/work/agoric-sdk/agoric-sdk/node_modules/@agoric/babel-standalone/babel.js:68362: error: \\[pP]{ invalid quantifier!

line 68362:

      if (hasFeature$1(features, FEATURES$1.unicodePropertyEscape) && /\\[pP]{/.test(pattern)) {
        unicodePropertyEscape = true;
      }

I'm not sure what's going on with them, they don't look particularly unusual to me.

Ideally/eventually, XS would be fixed to accept these. A shorter-term solution might be for us to rewrite/replace them in this (@agoric/babel-standalone) fork. Such a change may or may not be appropriate for pushing upstream.

cc mainly @michaelfig and @phoddie , also @FUDCo

@warner
Copy link
Author

warner commented Jul 2, 2020

The first is coming from source-map (we get version 0.6.1, but current trunk does the same thing):

https://github.com/mozilla/source-map/blob/ac518d2f21818146f3310557bd51c13d8cff2ba8/lib/util.js#L432-L434

The second comes from regjsparser:

https://github.com/jviereck/regjsparser/blob/1325cad7b3dc22d8a9ea88136e5c424f0d4b7a11/parser.js#L628-L636

The third comes from babel itself:

if (flagsIncludesU) {
if (!hasFeature(features, FEATURES.unicodeFlag)) {
useUnicodeFlag = true;
}
if (
hasFeature(features, FEATURES.unicodePropertyEscape) &&
/\\[pP]{/.test(pattern)
) {
unicodePropertyEscape = true;
}
}

The fact that two are coming from upstream libraries means this will be a lot harder to patch locally, we'd have to fork both of those libaries too.

@phoddie
Copy link

phoddie commented Jul 2, 2020

Your call on how to deal with this in your code. My advice would be to wait a day or two to give us time to investigate. There's some chance it is an easy fix.

@warner
Copy link
Author

warner commented Jul 2, 2020

Cool, thanks!

@warner
Copy link
Author

warner commented Jul 3, 2020

cc @dckc

@phoddie
Copy link

phoddie commented Jul 7, 2020

Update. These RegExps are not standard JavaScript. ;) They rely on Annex B.

We have updated XS to support this particular Annex B behavior. We'll push that in the coming days.

Still, none of these projects need to rely on Annex B. They likely aren't even aware of the dependency. The RegExps can (even should) be updated to eliminate the dependency.

@dckc
Copy link

dckc commented Aug 4, 2020

@phoddie thanks for the fix!

Moddable-OpenSource/moddable#383 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants