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

The "initializer" property of a descriptor for a plain object #41

Closed
fatfisz opened this issue Oct 11, 2015 · 17 comments
Closed

The "initializer" property of a descriptor for a plain object #41

fatfisz opened this issue Oct 11, 2015 · 17 comments

Comments

@fatfisz
Copy link

fatfisz commented Oct 11, 2015

Just for the record: while writing this I came to a conclusion that "all" of my problems would be solved if I only have used the decorators in the Python way, i. e. only for classes and methods, and not for properties. I'm still leaving my thoughts though, as maybe they'll prove useful in one way or the other.

I have a problem with the temporary initializer descriptor property.

I wanted to decorate a property of a plain object and found out that babel is doing something which seemed strange to me. The value descriptor property was nowhere to be found when my decorator was called, and instead I've found an initializer property on the descriptor. That led me to this spec and the spec about class fields. After finding no clues as to why should that property even exist when I was dealing with a plain object, I've first created an issue in the babel repo, and then turned here.

I'll now present my thoughts on the matter of the issue. Please be gentle with me.

First, let's look at a simple object.

let person = {
  born: Date.now()
};

If we'd really like to use the Object.defineProperty function to highlight what's going on with the descriptor, it would look like this:

let person = {};
Object.defineProperty(person, 'born', {
  value: Date.now(),
  enumerable: true,
  writable: true,
  configurable: true,
};

I don't really see a reason to introduce another descriptor property here.

Now, the way a decorator would handle this if we didn't know classes existed is quite straightforward - just do the same thing as with the methods. But, enter classes - and boom, everything has to be changed. I know that in reality we have classes, but I wanted to take you to a different world for a second to provide another perspective.

Let's take a look at the classes now.
I've read the es-class-static-properties-and-fields spec and didn't find any clues to the initializer property. That's why I was puzzled by the example in INITIALIZER_INTEROP.md from this repo:

class Person {
  born = Date.now();
}

// desugars to roughly:

class Person {
  constructor() {
    let _born = Object.getPropertyDescriptor(this, 'born');
    this.born = _born.initializer.call(this);
  }
}

Object.defineProperty(Person.prototype, 'born', {
  initializer() { return Date.now() },
  enumerable: true,
  writable: true,
  configurable: true,
});

I thought that doing it this way didn't match what was described in the class fields spec. So I've decided to try and do it in another way:

class Person {
  constructor() {
    Object.defineProperty(this, 'born', {
      value: initializer.call(this),
      enumerable: true,
      writable: true,
      configurable: true,
    });
  }
}

let initializer = function () {
  return Date.now();
};

This solution doesn't clutter the prototype and otherwise behaves like the original solution - has the proper scoping and the initializer is called properly. Then I tried adding a decorator to my solution:

class Person {
  @readonly
  born = Date.now();
}

// is desugared to

class Person {
  constructor() {
    let desc = {
      value: initializer.call(this),
      enumerable: true,
      writable: true,
      configurable: true,
    };
    desc = readonly(this, 'born', desc) || desc;
    Object.defineProperty(this, 'born', desc);
  }
}

let initializer = function () {
  return Date.now();
};

Things here differ from the original solution a bit more than in the previous situation:

  • the decorator is called with this as a target and not with a prototype - that seems more correct to me, as the property is being assigned to the instantiated object and not the prototype, so this seems more proper as a "target"
  • the decorator is applied each time the constructor is ran, instead of applying it only once. But then the field that is decorated isn't "applied" only once - its expression is evaluated each time an object is instantiated, so the decorator behavior is more in sync with what's happening with the field

I think that decorators should operate on the standard descriptor because it's possible, as I've demonstrated above.

Those are my honest thoughts. If there was already a similar discussion about these problems, please point me there.

@silkentrance
Copy link

Please close, outdated/invalid/personal learning process.

@fatfisz
Copy link
Author

fatfisz commented Feb 29, 2016

@silkentrance Well, sorry for being a "frickin' bugger", but it seems you didn't even read and understand what I've written here.

This is not outdated, because initializer.md still contains info that is incompatible with the "ES Class Fields & Static Properties" spec. Nothing in there is written about the initializers being accessible as enumerable properties on the prototype nor about the strange "initializer" descriptor method or its use in the presented way.

Saying that something is invalid without providing any arguments is just rude.

And while this contains something that could be called by some stretch a "personal learning process", is that wrong? I presented some problem, and you're not addressing its matter.

The problem is that the "value" of the property is plainly ignored while being handled in the proposed way - even if a decorator changes it, in the end it's overwritten. I provided a solution for that, so it would be a bit nicer to discuss it.

@silkentrance
Copy link

@fatfisz Well, this is because these initializers are a special case and will be evaluated during constructing an instance of a class. As such, the so initialized properties will be part of the instance and thus be enumerable and what not, for example the below code is similar to what is getting transpiled

function MyClass() {
     // this is an initialized property
     this.initialized = (function callingInitializerHere() { return 5; })();
}

based upon

class MyClass {
     initialized = 5;
}

And if you find this rude, please be my guest and join the Linux developer community 😀
And, yes, this was a joke.

So. please close as it is in fact invalid and based upon a steep personal learning curve.

And in https://github.com/jeffmo/es-class-fields-and-static-properties#part-1-class-instance-fields you have the prove, albeit lest the actual implementation of it.

@fatfisz
Copy link
Author

fatfisz commented Feb 29, 2016

@silkentrance I agree that the properties will be enumerable on the instance, but in the interop code they are enumerable on the prototype. I presented an alternative that gets rid of that and also gets rid of the unnecessary crippling of the decorators. In the first place, class fields were proposed as a nicer way of initializing properties in the constructor (as you've shown yourself), so moving the Object.defineProperty outside of the constructor in the desugared code is closer to being invalid than what I proposed.

Again, you didn't show where my thinking is invalid. I can only conclude that you assumed that I know neither what descriptors are nor what the class fields proposal is about, and are basing your "this is invalid" statement on that. Please, first read what I've written about and then comment. You'd do better presenting a some reasoning for the current interop proposition and any arguments against mine, which you have yet to do.

I know what Linux dev community is known for, but they are usually mentioned as a bad example. Maybe we should strive for something more?

@silkentrance
Copy link

@fatfisz first, you get me wrong. second, you should work on your self-awareness, knowing what you are capable of. third, this is neither the time nor place to discuss psychology, and, yes, this was a yoke. fourth, I will check right away whether I am wrong in my assumption using babel 6.5.1 and whether you are correct in your assessment.

And last but not least, I want to get things rolling here again as it seemingly came to a halt and we definitely need these decorators in babel 6 without the help of the interim plugin.

@silkentrance
Copy link

@fatfisz after having reviewed you OP once again, I came up with this

Source:

class Person {
    lessonLearned = true
}

class DecoratedPerson {
    @withASteepLearningCurve
    lessonLearned = true
}

// the decorator
function withASteepLearningCurve() {}

Transpiled Source

"use strict";

import _Object$defineProperty from "babel-runtime/core-js/object/define-property";

var _classCallCheck2 = require("babel-runtime/helpers/classCallCheck");

var _classCallCheck3 = _interopRequireDefault(_classCallCheck2);

var _desc, _value, _class2, _descriptor;

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

function _initDefineProp(target, property, descriptor, context) {
    if (!descriptor) return;

    _Object$defineProperty(target, property, {
        enumerable: descriptor.enumerable,
        configurable: descriptor.configurable,
        writable: descriptor.writable,
        value: descriptor.initializer ? descriptor.initializer.call(context) : void 0
    });
}

function _applyDecoratedDescriptor(target, property, decorators, descriptor, context) {
    var desc = {};
    Object['ke' + 'ys'](descriptor).forEach(function (key) {
        desc[key] = descriptor[key];
    });
    desc.enumerable = !!desc.enumerable;
    desc.configurable = !!desc.configurable;

    if ('value' in desc || desc.initializer) {
        desc.writable = true;
    }

    desc = decorators.slice().reverse().reduce(function (desc, decorator) {
        return decorator(target, property, desc) || desc;
    }, desc);

    if (context && desc.initializer !== void 0) {
        desc.value = desc.initializer ? desc.initializer.call(context) : void 0;
        desc.initializer = undefined;
    }

    if (desc.initializer === void 0) {
        Object['define' + 'Property'](target, property, desc);
        desc = null;
    }

    return desc;
}

function _initializerWarningHelper(descriptor, context) {
    throw new Error('Decorating class property failed. Please ensure that transform-class-properties is enabled.');
}

var Person = function Person() {
    (0, _classCallCheck3.default)(this, Person);
    this.lessonLearned = true;
};

var DecoratedPerson = (_class2 = function DecoratedPerson() {
    (0, _classCallCheck3.default)(this, DecoratedPerson);

    _initDefineProp(this, "lessonLearned", _descriptor, this);
}, (_descriptor = _applyDecoratedDescriptor(_class2.prototype, "lessonLearned", [withASteepLearningCurve], {
    enumerable: true,
    initializer: function initializer() {
        return true;
    }
})), _class2);

// the decorator
function withASteepLearningCurve() {}

So what you are actually complaining about is

}, (_descriptor = _applyDecoratedDescriptor(_class2.prototype, "lessonLearned", [withASteepLearningCurve], {
    enumerable: true,
    initializer: function initializer() {
        return true;
    }
})), _class2);

where the lessonLearned property is declared on the prototype as well?

Why shouldn't it be?

See

https://github.com/jeffmo/es-class-fields-and-static-properties#instance-field-declaration-process

amd later on it states that for

When an instance field with an initializer is specifed on a non-derived class (AKA a class without an extends clause), the list of fields (each comprised of a name and an initializer expression) are stored in a slot on the class object in the order they are specified in the class definition. Execution of the initializers happens during the internal "initialization" process that occurs immediately before entering the constructor.

and the same holds true for instance fields with initialized on derived classes

In the spec it reads just before entering the constructor. So, and since babel is not an engine but just a transpiler, I strongly believe that babel is correct in that it will apply the initialization inside the constructor. And defining these properties afterwards may not work since other behavior of the same class or might depend on these properties.

Besides of that, the property gets defined on the this instance and not the prototype.

As such, the implementation is correct.

@silkentrance
Copy link

What actually bothers me more is the inclusion of the unused code

function _initializerWarningHelper(descriptor, context) {
    throw new Error('Decorating class property failed. Please ensure that transform-class-properties is enabled.');
}

@fatfisz
Copy link
Author

fatfisz commented Feb 29, 2016

@silkentrance If you're talking about self-awareness, then I hope you know you've previously provided a bad example, because that's not how the scoping works with field initializers. They shouldn't have the scope of the constructor as their parent scope, but the scope the class is in.

Jokes can be rude too - and please, let's leave it at that.

Babel is not a source of truth. In fact, I came here because Babel (at the time 5.x) was doing something unreasonable. This proposal is the place to discuss how things should work and the only role Babel has here is to follow the spec.

Let me just say, that I really appreciate you wanting to move things that have been stale for a couple of months. It's just the way you are going about it that I don't like.

Thank you for responding with some code (nice choice of names, haha), I will respond to it in the next comment.

@silkentrance
Copy link

@fatfisz as a personal note and based upon personal experience: https://en.wikipedia.org/wiki/Vitamin, have a fish or two a month B* FTW

@fatfisz
Copy link
Author

fatfisz commented Feb 29, 2016

So I think that the prototype should be left alone because the fields are concerned only with the constructor. You've cited:

the list of fields (each comprised of a name and an initializer expression) are stored in a slot on the class object

So there's no need to clutter the prototype. If anything, it is just confusing, because properties from the constructor are suddenly dragged ut to the prototype.

I agree that if we leave the prototype out, the interop (and Babel too) achieve what should be achieved, at least with the initialization of the fields. But if we add decorators to that, then there is suddenly a difference between how methods and fields are treated:

  • methods can have their value changed by a decorator, and this change will be preserved in the initialized instance
  • fields can have their value changed by a decorator, but in the end the interop (and Babel) overshadow it by setting it to the result of calling the initializer

This was a big surprise to me and I think it's a bad design. Let's look once again what happens now with fields:

  1. Initialize the descriptor without the value
  2. Apply decorators to it
  3. Set the value to the result of calling the initializer

And this is what I propose:

  1. Initialize the descriptor with the value set to the result of calling the initializer
  2. Apply descriptors to it

This way the methods and fields will be treated uniformly.

It is possible to achieve that even if we clutter the prototype as it is done now, but the whole _applyDecoratedDescriptor thing would have to be moved inside the constructor and not outside of it.

I think that the idea behind the current interop code was to compute the field decorator only once and that's where the difference stems from.

@fatfisz
Copy link
Author

fatfisz commented Feb 29, 2016

@silkentrance Once you go personal you can't go back, eh? We clearly have a different sense of humor, why can't you just understand that? If "this is neither the time nor place to discuss psychology", then I think it's neither a place to discuss medicine nor eating habits.

@silkentrance
Copy link

@fatfisz consider

Either way, when decorating, big surprise, the decorator is expected to either completely replace or otherwise alter the decorated class, method or property. Including also, completely replacing the initializer.

And this is by design and we, as the developers of frameworks of all kinds of sorts, definitely need it.

As for

And this is what I propose:
Initialize the descriptor with the value set to the result of calling the initializer
Apply descriptors to it

Descriptors are simple objects, having a few properties but no logic. Decorators on the other hand are full of (framework) logic and will replace either the initializer property or the getter or setter or the value or even the target with whatever the framework deems provable.

As such, please try to implement a suite of decorators first. Might I suggest you digging into inxs-common to get a feel for these decorators and how these are implemented inxs?

A better example yet would be core-decorators.js since that one is actually being used in productive code whereas the above examples are not.

@fatfisz
Copy link
Author

fatfisz commented Feb 29, 2016

@silkentrance So yes, I was right about your assumptions. You actually said that I should first try to play with decorators, when in fact I've been using them even before this proposal was created.

And when it comes to JS, this has been used in production for almost half a year now:

export default createProcessor({
  @decorators.from('postId')
  @decorators.assert.not.isNull('Missing post id')
  @decorators.assert.isMongoId('Invalid id')
  @decorators.transform(ObjectID)
  @decorators.assign
  _id() {},

  @decorators.assert.not.isNull('Missing index')
  @decorators.assert.isInt('Invalid index', { min: 0 })
  @decorators.transform(Number)
  @decorators.assign
  index() {},
});

Making the initializer into a part of the descriptor is not some holy, untouchable solution. It is a hack, and the class field spec doesn't include anything like that.

@silkentrance
Copy link

@fatfisz none of your examples make use of the initializer, which is only available when

class Decorated {
  @decorate
  foo = "bar"
}

However, I am tired of discussing this as it is clearly mentioned in the official spec, as I pointed out before.

@fatfisz
Copy link
Author

fatfisz commented Mar 2, 2016

@silkentrance Putting initializers in descriptors is not described in the class fields spec, only in this one, so this is a bit backwards.

None of my examples "make use" of the initializer, because I consider its existence invalid. The whole point of my OP was that I'd like to have a simple access to the value, not to some workaround thingie. I did however mention the initializer here:

class Person {
  @readonly
  born = Date.now();
}

The post came to be because now there is a difference between

{
  @foo
  bar() {}
}

and

{
  @foo
  bar: function() {}
}

And I deem it unnecessary.

Getting tired? Good riddance, I was getting tired of your creepiness and trolling too.

@silkentrance
Copy link

@fatfisz well, it does state

When an instance field with an initializer is specifed on a non-derived class (AKA a class without an extends clause), the list of fields (each comprised of a name and an initializer expression) are stored in a slot on the class object in the order they are specified in the class definition. Execution of the initializers happens during the internal "initialization" process that occurs immediately before entering the constructor.

When an instance field with an initializer is specified on a derived class (AKA a class with an extends clause), the list of fields (each comprised of a name and an initializer expression) are stored in a slot on the class object in the order they are specified in the class definition. Execution of the initializers happens at the end of the internal "initialization" process that occurs while executing super() in the derived constructor. This means that if a derived constructor never calls super(), instance fields specified on the derived class will not be initialized since property initialization is considered a part of the SuperCall Evaluation process.

So the initializer is definitely being mentioned as an expression that is stored alongside the slot of the declaring class. And the slot BTW is the descriptor and the initializer expression, in your example, is then a function that returns Date.now(), e.g. function () { return Date.now() }.

Now this being out of the way, I definitely think that this is according to the specification and that this issue is moot and can be closed right away.

@fatfisz
Copy link
Author

fatfisz commented Apr 19, 2016

Closed in favor of #71.

@fatfisz fatfisz closed this as completed Apr 19, 2016
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

2 participants