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

Making public data property initializations more user-friendly #5

Open
mbrowne opened this issue Nov 4, 2018 · 12 comments
Open

Making public data property initializations more user-friendly #5

mbrowne opened this issue Nov 4, 2018 · 12 comments

Comments

@mbrowne
Copy link

mbrowne commented Nov 4, 2018

The readme states:

Public data properties are placed on the prototype and initialized with undefined if no initializer is given, or the value of the initializer at the time the class definition is evaluated.

Given this behavior, I suggest that the use of this in a public data property initialization should be an error. This could help reduce confusion among developers, especially if their expectation is that these kinds of declarations create instance properties.

@mbrowne
Copy link
Author

mbrowne commented Nov 4, 2018

And I suggest that the use of this would also be an error in the case of instance variable initializers, given this:

Instance variable definitions may have initializers. [...] The value of an initializer is determined at the time the class definition is parsed. Instance-specific value assignments can only be performed in the constructor.

@ljharb
Copy link

ljharb commented Nov 4, 2018

That means initializers wouldn’t be able to call instance methods, which would impede refactoring.

@mbrowne
Copy link
Author

mbrowne commented Nov 4, 2018

@ljharb Per-instance initializers would be my preference, but those don't exist in this proposal. So I assume that if you did try to use this, it would either be undefined or point to the class rather than the instance, since according to this proposal initializers are evaluated at class creation time...In which case I think it's better not to allow use of this at all.

@mbrowne
Copy link
Author

mbrowne commented Nov 4, 2018

Backing up for a moment, @rdking I understand your reasoning for wanting to avoid per-instance initializers for properties, but why does the rule also apply to private instance variables?

@rdking
Copy link
Owner

rdking commented Nov 4, 2018

There's one other source of this, the current execution context. If this exists when the class definition is being evaluated, then it is fair for property initializers to use this. Where private instance variables are concerned, since those are "transported" to closures in their entirety, the value of this will always be the current instance object.

I concede that this leads to a potentially confusing inconsistency and am very open to arguments that say not to allow it. At the same time, I'm open to any argument saying to should be allowed. I'm marginally partial to allowing it despite the potential confusion because not allowing would be removing a capability that comes naturally with execution contexts.

@rdking
Copy link
Owner

rdking commented Nov 4, 2018

Let me be a little more clear. Property declarations will not have any access to a this referring to the class itself or an instance of the class because neither of those things exists at the time of class definition evaluation. Property declarations can, however, make use of this if the current execution scope has a this value to work with.

On the other hand, instance variables can make use of this since the current execution context for their evaluation is the instance or class closure they are contained in. Since that closure always has a this value corresponding to either the class or an instance of it, there are no issues with assigning instance variables a this-based, instance specific value.

If this isn't clear from either the README.md or /docs/documents.md files, let me know so I can fix it.

@mbrowne
Copy link
Author

mbrowne commented Nov 4, 2018

I think the readme is clear about properties, but not instance variables. Here's the quote again, referring to instance variables:

The value of an initializer is determined at the time the class definition is parsed. Instance-specific value assignments can only be performed in the constructor.

You seem to be saying something very different here.

@rdking
Copy link
Owner

rdking commented Nov 4, 2018

I've actually been considering both sides of the issue. My first approach was to say that everything in the definition is entirely static. When @ljharb finally came forward with the use-case for wanting instance-properties, that was enough to tip my opinion the other way. There's no reason that I can come up with for not allowing the whole instance variable/constant (initializer included) to be transported as-is into the corresponding closure. Allowing that provides another possible solution to airbnb's need while still avoiding the problematic instance-properties.

@mbrowne
Copy link
Author

mbrowne commented Nov 4, 2018

I concede that this leads to a potentially confusing inconsistency and am very open to arguments that say not to allow it. At the same time, I'm open to any argument saying to should be allowed.

If I understand your current thinking correctly, this potential confusion is only an issue for (non-static) public properties, not instance variables. In any case... Aside from my personal opinions, I would just recommend that anytime something can be changed about this proposal to avoid something that could be majorly confusing, that should probably outweigh other considerations. Leaving potential foot-guns in place will reduce the chances of this proposal being accepted by the larger community and committee at this point to even lower than it seems they already are. And after all, isn't avoiding foot-guns one of the major reasons for offering this proposal as an alternative to class fields?

@rdking
Copy link
Owner

rdking commented Nov 4, 2018

...isn't avoiding foot-guns one of the major reasons for offering this proposal as an alternative to class fields?

Not really. That's just an added bonus. I want to avoid adding something to the language that will force everyone to walk on eggshells around it as soon as someone shares code that uses it. That's what class-fields will cause. My goal with this proposal is to provide something that allows reasonably close to equivalent language features for the same work without breaking as few as possible of any of the reasonable expectations of the developers.

I would just recommend that anytime something can be changed about this proposal to avoid something that could be majorly confusing, that should probably outweigh other considerations.

The hard part about that is "majorly". By myself, I'm no where near capable of determining what most people will find majorly confusing. I'd want more people to chime in and say the same. The problem is that on public properties, this will refer the execution context of the current function. For instance variables, this will refer to the instance that the instance or class closure is attached to. I could understand disallowing this on property declarations, but I can't remove it for instance variables.

@mbrowne
Copy link
Author

mbrowne commented Nov 4, 2018

I could understand disallowing this on property declarations, but I can't remove it for instance variables.

Well it sounds like there's no need to in the case of instance variables, if it would simply work as expected.

@rdking
Copy link
Owner

rdking commented Dec 20, 2018

As of the current updates, the use of "this" in the declarations still isn't an error. However, it's no longer a necessity unless there is local variable sharing the same name as the class member being used.

class X1 {
  let x = 2;
  let y = x**2; //Perfectly valid, but order matters.
}

var r = "foo";
class X2 {
  let r = 7;
  let bad = (4*Math.PI*r**2)/3; //Won't work because r == "foo"
  let a = (4*Math.PI*this::r**2)/3; //Works

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

3 participants