Give correct RegExp flags in toString

[bz2]

When the flags property is not available on RegExp, the internals regexp-flags implementation should be used regardless of DESCRIPTORS.

Fixes: #536

I'm not certain this is the right fix, but the value of DESCRIPTORS is true on IE 11 and the internals implementation would be fine - and doesn't seem to depend on defineProperty.

Also not sure how to include a focused test, but the linked issue has a project that reproduces the problem.

[zloirock]

I think that there should be something like

var R = anObject(this);
var p = String(R.source);
var rf = R.flags;
var f = String(rf === undefined && R instanceof RegExp && !('flags' in RegExpPrototype) ? flags.call(R) : rf);
return '/' + p + '/' + f;

[bz2]

@zloirock Thanks for looking over this. I've read these parts of the spec now so have a slightly better understanding.

In your suggested implementation, I'm not sure I understand the purpose of checking RegExpPrototype. Any conforming flags getter cannot return undefined, only raise a TypeError or return a string. So, if rf is not a string but R is a RegExp instance the code should always use the internals flags implementation instead?

https://www.ecma-international.org/ecma-262/6.0/#sec-get-regexp.prototype.flags
https://www.ecma-international.org/ecma-262/6.0/#sec-regexp.prototype.tostring

On the tests, I have a plan that would sort of work, but is likely too big a change for this branch. Main issue is the tests are always run in a browser context that includes the whole polyfill bundle:

tests/tests.html:

<script src='../packages/core-js-bundle/index.js'></script>
<!-- ... -->
<script src='./bundles/tests.js'></script>

It would be possible to change .webpack.config.js for instance, to have an entry per test file, rather than one for all tests:

  tests: {
    entry: './tests/tests/index.js',
    output: { filename: 'tests.js' },
  },

That would then make it possible to test each file with only its own polyfill rather than all of core-js. That still doesn't help if the tests are run on a modern chrome where the feature is implemented anyway, but is heading in a sensible direction?

[zloirock]

In your suggested implementation, I'm not sure I understand the purpose of checking RegExpPrototype...

It can be called on non-regex or a RegExp subclass instances. As you can see in the spec, those methods are generics. flags property can be redefined. Internal implementation can't be used always since it could have side effects in the same cases. So in my code above you can see an example of correct fallback for engines without RegExp.prototype.flags.

[zloirock]

You have correctly identified a problem of testing. However, the problem is deeper than you think - for example, a big part of modules are not independent and should work in different ways in different combinations and those combinations defined in many different places like CommonJS entry point, @babel/preset-env definitions, etc.

[bz2]

@zloirock Have poked around and can't come up with a scenario that doesn't work correctly with the in instance check and instanceof. Plain objects use .flags, only things with RegExp in the prototype chain can get the internals implementation, and it doesn't really seem possible to subclass RegExp anyway? If you can give an example test case that fails, I'll adapt the logic.

[zloirock]

@bz2 it's not a problem to write an example with usage of Proxy where we can redefine in behavior on R. However, it's also required for consistency with other cases like this.

[bz2]

@zloirock Have updated with basic proxy test (that would have failed on non-flags browsers only) and that spelling. Does seem like the same form should be used in match-all and here, but that code is currently not caring about being passed non-RegExp objects?

[zloirock]

@bz2 since we can't polyfill Proxy and early Proxy implementations have inconsistent API, I don't think that adding a test with Proxy makes sense. Sure, match-all should be updated, but in the scope of #516. I think I will do it myself.