fix($injector): fix class detection RegExp

[gkalpak]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
ES6 classes are not properly detected unless class is followed by a whitespace character.

What is the new behavior (if this is a feature change)?
ES6 classes are properly detected, even if class is not followed by a whitespace character.
E.g. class{} or class/* ... */ {}.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

• The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

Other information:
Mentioned in #14531 (comment).

[mgol]

This line seems kind of moot now.

[gkalpak]

Removed

[gkalpak]

There seem to be some failures on Safari. I'll have to investigate further 😒

[gkalpak]

So, basically, we can't detect classes in Safari 9 😞
This is a known issue (see e.g. #13785).

What should we do about it ? Skip the tests on Safari ?

[mgol]

What should we do about it ? Skip the tests on Safari ?

Yeah, I think so. Classes are not supported everywhere yet, it's natural some browsers might have buggy support for some time; people relying on classes in Angular will need to restrict support to well-behaving environments.

Note that Safari Technology Preview stringifies classes correctly so Safari 10 (coming out in ~6 months if the past is any indication) will be fine.

[gkalpak]

Yeah, I meant what should we do about the tests (which are failing on CI - as expected).
I will restrict them to Chrome for now (unless someone has a better idea).

[mgol]

@gkalpak Why just to Chrome? What about Firefox?

[gkalpak]

Firefox (at least up to v46) does not stringify classes in a way that allows class detection 😞

> (class Test { constructor(a) { this.a = a; } aMethod() { return this.a; } }).toString()
> "function Test(a) {
  "use strict";
   this.a = a; }"

[mgol]

Yeah, they have an open bug for it, it's not fixed even on Nightly (v49) yet: https://bugzilla.mozilla.org/show_bug.cgi?id=1216630

[dcherman]

Why wrap this in a browser sniff for Chrome only? Since this test doesn't look browser specific and is just more about the injector doing the right thing, I think it'd make more sense to feature detect the support for ES6 classes

  var supportsClasses = (function() {
    try {
      var klass = eval("class C {}");

      // Workaround the buggy implementation of `toString` in Firefox
      // https://bugzilla.mozilla.org/show_bug.cgi?id=1216630
      return klass.toString().indexOf('class') !== -1;
    } catch (err) {
      return false;
    }
  }());

  if (supportsClasses) { ... }

[mgol]

The situation in tests is a little different; in jQuery e.g. we don't sniff in source but if a specific bug exists in a specific browser & version we do sniff in tests. This is not only because it's simpler to do but also to rule out the possibility that a broken support check will make the tests be skipped - we do want to verify that in these specific browsers they're being run.

I agree with you here, though as we're talking about a bleeding-edge feature that browsers are only implementing now and more & more of them will have it in a working state. That's different.

[gkalpak]

I generally agree, but there is not much we can do here unfortunately. There is more context in the comments (e.g. starting at #14533 (comment)).