It may be easier to have back and forth if this is converted into a temporary PR. That way we can do line comments.
- why error when private identifiers are used in JSX tag names? The following is allowed today, I don't see why private identifiers wouldn't be allowed as well.
class A {
private foo = () => null
constructor {
const z = <this.foo />
}
}- The changes we're working on alter the error message, but that's not exactly what Ron was asking for:
Do we need downleveled here? If it is situational (depending on compiler flags/target), perhaps we should have a separate diagnostic message and chose which one to report based on whether we are downleveling private identifiers.
I propose the following and am happy to implement the change:
- Keep 'downleveled' in the error message
- If the target is ESNext, don't error on private fields here.
- We can reply to Ron along these lines:
If I understand correctly, it only makes sense to error in this case when we are downleveling. So we propose keeping the error message as-is, but only erroring when the target is not ESNext.
It also appears that when TS supports not downleveling public fields with initialiers that we may need to add similar logic so that we don't error on things like this:
class A extends B {
foo = 3;
constructor () {
this.foo = 5;
super();
}
}- Re the "Fix private identifier ++/-- numeric coercion" commit:
Most of the diff seemed to be refactoring unrelated to the bug fix. Am I reading it correctly? If so, we might want to not do the refactor in order to make the change easier for Ron to review.
I find the new createCopiableReceiverExpr elegant and easy to read. The return type doesn't seem to be idiomatic to the TS codebase, however:
: [Expression, Expression | undefined]I couldn't find other cases of a function returning a single tuple. There are several cases in the checker where an object is used to imitate multiple returns. How about:
: { readExpression: Expression; initializeExpression: Expression}- Re Ron's feedback to specify multiple test targets like this:
es5, es2015: I think he wanted this change to be applied more widely. What do you think? It would be good to review the baseline changes together, as this will be a big diff!
RE erring on JSX tag names: the JSX specification shows that the tag name can only be a JSXIdentifier or a JSXMemberExpression (where the property name can only be an JSXIdentifier), so arbitrary expressions are not allowed here. I didn't think we should deviate from that specification.
RE the altered error message: I altered the error message because I don't think the error message needs to be dependent on the target. We always move the initializations of private fields to the constructor because we have to preserve evaluation order with existing class fields (and those need to continue to be transformed as assignments in the constructor), so this error message needs to be emitted in the same situations as initialized public fields.
I think in your example, it would still be an error to do
this.foo = 5;before the super call with the ES class fields, although the error would probably say'super' must be called before accessing 'this' in the constructor of a derived class.and it wouldn't emit this error. So it's a fair point that this error will become target dependent in the future, but for now I think we can just always emit it.Good point about the test targets - I wonder if they want to find some balance with the number of targets they emit with, since adding more targets adds performance, maintenance, and review time penalties. Should we reach out to Ron and confirm that they want these targets in all the private name tests (or if some tests gain more value from the targets than others)?