Support inheritance of properties which are not enumerable#2375
Support inheritance of properties which are not enumerable#2375ChadKillingsworth wants to merge 2 commits intogoogle:masterfrom
Conversation
I cannot describe the feeling of doom and dread that comes over me when I read this phrase. |
|
@MatrixFrog You might as well get past that. |
|
Why would someone want to use two different transpilers in the same JS program? |
|
Because closure-compiler specifically doesn't support some valid ES6 transpilation OR the code is distributed pre-transpiled. |
|
Okay. Well in general I would expect Babel to make rather different choices from us, so I'm surprised it works at all and would generally advise people against depending on it always working. |
|
I agree - except Babel is correct here and we aren't :) |
e6989db to
09c41c4
Compare
|
So apparently there were some tests. I've fixed those and the change should now be much safer. |
09c41c4 to
b574d2f
Compare
| var propNames = Object.getOwnPropertyNames(parentCtor); | ||
| for (var i = 0; i < propNames.length; i++) { | ||
| var descriptor = Object.getOwnPropertyDescriptor(parentCtor, propNames[i]); | ||
| if (descriptor && (descriptor.enumerable || (descriptor.writeable && descriptor.configurable))) { |
There was a problem hiding this comment.
I misspelled writable here. Update incoming...
b574d2f to
c60c268
Compare
|
Updated to correct the spelling of |
c60c268 to
f960905
Compare
| var propNames = Object.getOwnPropertyNames(parentCtor); | ||
| for (var i = 0; i < propNames.length; i++) { | ||
| var descriptor = Object.getOwnPropertyDescriptor(parentCtor, propNames[i]); | ||
| if (descriptor && (descriptor.enumerable || descriptor.configurable))) { |
There was a problem hiding this comment.
I believe travis is failing, because there's one to many closing parentheses on this line.
45f49da to
5bb9deb
Compare
|
I fixed the extra parenthesis and squashed/rebased the result. |
|
Sorry about that - I'm juggling too many branches right now. |
|
@brad4d This isn't quite right - there are edge cases that just aren't working. I'm trying to find a combination that works properly. |
| if (Object.defineProperties) { | ||
| var propNames = Object.getOwnPropertyNames(parentCtor); | ||
| for (var i = 0; i < propNames.length; i++) { | ||
| var descriptor = Object.getOwnPropertyDescriptor(parentCtor, propNames[i]); |
There was a problem hiding this comment.
To use getOwnPropertyDescriptor here, I believe you need to add a require above.
'require es6/object/getownpropertysymbols';
There was a problem hiding this comment.
I may be wrong about this. When I added the 'require' locally to see if it would fix our failing internal test it just added more errors about stuff defined in getownpropertysymbols.js.
Perhaps just adding to the explict externs in the test classes as you've done is the way for me to fix that.
@shicks how do 'require' lines work in these files?
Should they all explicitly require anything they use from other file,
or is that not needed for things declared in externs?
There was a problem hiding this comment.
@brad4d My understanding is the require statements are needed for methods which you can polyfill. However there is no polyfill for getOwnPropertyNames for ES3. So in this case I didn't import them. Object.defineProperties is the same way - no ES3 equivalent.
|
@ChadKillingsworth What problems are you referring to when you say this isn't quite right yet? |
|
What I had only worked with a single level of inheritance. Multiple levels failed. I think the latest commit fixes it. Also, here's a full test case. Can we add this to the runtime tests? Since they don't run in the open source project it's hard for me to do that. var _createClass = function () { function defineProperties(target, props) { for (var i = 0; i < props.length; i++) { var descriptor = props[i]; descriptor.enumerable = descriptor.enumerable || false; descriptor.configurable = true; if ("value" in descriptor) descriptor.writable = true; Object.defineProperty(target, descriptor.key, descriptor); } } return function (Constructor, protoProps, staticProps) { if (protoProps) defineProperties(Constructor.prototype, protoProps); if (staticProps) defineProperties(Constructor, staticProps); return Constructor; }; }();
function _possibleConstructorReturn(self, call) { if (!self) { throw new ReferenceError("this hasn't been initialised - super() hasn't been called"); } return call && (typeof call === "object" || typeof call === "function") ? call : self; }
function _inherits(subClass, superClass) { if (typeof superClass !== "function" && superClass !== null) { throw new TypeError("Super expression must either be null or a function, not " + typeof superClass); } subClass.prototype = Object.create(superClass && superClass.prototype, { constructor: { value: subClass, enumerable: false, writable: true, configurable: true } }); if (superClass) Object.setPrototypeOf ? Object.setPrototypeOf(subClass, superClass) : subClass.__proto__ = superClass; }
function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }
var Base = function () {
function Base() {
_classCallCheck(this, Base);
}
_createClass(Base, null, [{
key: 'origMethod',
value: function origMethod() {
return 'hello world';
}
}]);
return Base;
}();
var Intermediate = function (_Base) {
_inherits(Intermediate, _Base);
function Intermediate() {
_classCallCheck(this, Intermediate);
return _possibleConstructorReturn(this, (Intermediate.__proto__ || Object.getPrototypeOf(Intermediate)).apply(this, arguments));
}
_createClass(Intermediate, null, [{
key: 'intermediateMethod',
value: function intermediateMethod() {
return 'intermediate';
}
}]);
return Intermediate;
}(Base);
class MyClass extends Intermediate {}
MyClass.intermediateMethod();
MyClass.origMethod(); |
|
As much as I hate to admit it, I think @MatrixFrog may be right here. I've come to conclude that the only correct way to handle this would be to use Instead, I'll just spend time fixing any transpilation blockers. |
Fix the ES6 inheritance polyfill to properly inherit properties which are not enumerable. This creates a problem when combining Closure-compiler transpiled code with Babel transpiled code. Babel sets ES6 static class methods as non-enumerable, but they should be inherited.
There are no open source tests so this change makes me somewhat nervous. Is
prototypethe only property that should be excluded?cc @shicks