Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($injector): fix class detection RegExp #14533

Closed
wants to merge 1 commit into from
Closed

fix($injector): fix class detection RegExp #14533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/auto/injector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,9 +851,9 @@ function createInjector(modulesToLoad, strictDi) {
}
var result = func.$$ngIsClass;
if (!isBoolean(result)) {
// Workaround for MS Edge.
// Check https://connect.microsoft.com/IE/Feedback/Details/2211653
result = func.$$ngIsClass = /^(?:class\s|constructor\()/.test(stringifyFn(func));
// Support: Edge 12-13 only
// See: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/6156135/
result = func.$$ngIsClass = /^(?:class\b|constructor\()/.test(stringifyFn(func));
}
return result;
}
Expand Down
20 changes: 20 additions & 0 deletions test/auto/injectorSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,26 @@ describe('injector', function() {
expect(instance.aVal()).toEqual('a-value');
});

if (/chrome/.test(navigator.userAgent)) {
Copy link
Contributor

@dcherman dcherman May 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) { ... }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)).

they('should detect ES6 classes regardless of whitespace/comments ($prop)', [
'class Test {}',
'class Test{}',
'class //<--ES6 stuff\nTest {}',
'class//<--ES6 stuff\nTest {}',
'class {}',
'class{}',
'class //<--ES6 stuff\n {}',
'class//<--ES6 stuff\n {}',
'class/* Test */{}',
'class /* Test */ {}'
], function(classDefinition) {
var Clazz = eval('(' + classDefinition + ')');
var instance = injector.invoke(Clazz);

expect(instance).toEqual(jasmine.any(Clazz));
});
}

// Support: Chrome 50-51 only
// TODO (gkalpak): Remove when Chrome v52 is relased.
// it('should be able to invoke classes', function() {
Expand Down