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

Start implementation of @let syntax #55848

Closed
wants to merge 3 commits into from
Closed

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented May 17, 2024

These changes are the beginning of the new @let syntax that will allow Angular developers to declare local variables in templates. The PR includes the necessary changes to the lexer, as well as the HTML and R3 AST nodes integration (see the individual commits). More PRs in the future will integrate the syntax into the various parts of the framework.

@let declarations are defined as:

  1. The @let keyword.
  2. Followed by one or more whitespaces.
  3. Followed by a valid JavaScript name and zero or more whitespaces.
  4. Followed by the = symbol and zero or more whitespaces.
  5. Followed by an Angular expression which can be multi-line.
  6. Terminated by the ; symbol.

Example usage:

@let user = user$ | async;
@let greeting = user ? 'Hello, ' + user.name : 'Loading';
<h1>{{greeting}}</h1>

Alternative syntaxes

While we were designing the syntax, we had some proposals for alternative syntaxes. This section covers why they weren't chosen over @let.

@const instead of @let

Most developers are taught to use const over let in TypeScript if the value doesn't change, and since @let can't be re-assigned, it could make sense to mirror the best practice from TypeScript. We've decided not to use @const because the value isn't actually constant, it is re-computed every time the template function runs.

A good mental model for how a @let variable works as part of change detection is this:

// Template
@let myVar = some.expression();
<button (click)="doSomething(myVar)">{{ myVar }}</button>

// Compiled
function MyCmp_Create() {
  // Creation
  let myVar;
  let button = document.createElement('button');
  button.addEventListener('click', () => doSomething(myVar));

  // Update closes over creation
  function MyCmp_Update() {
    myVar = some.expression();
    button.firstChild.textContent = myVar;
  }
}

Note how there is one instance of the variable myVar, which is closed over by the update operation. This pseudocode approximates how the variable's value is stored in the LView and used during change detection as well as accessed in event listeners.

The prohibition against writing to template variables is an extra restriction that Angular enforces, not an intrinsic property of the variable.

Entirely new keyword

We don't want to come up with a completely new keyword (e.g. @define), because we want to maintain familiarity with the TypeScript syntax.

@var instead of @let

Using var may make sense since the expression value is variable, but using var in JavaScript is considered an anti-pattern and we don't want to adopt the negative terminology in Angular.

var also implies different hoisting semantics which we want to avoid.

Block-like syntax

There was a proposal to introduce a syntax similar to blocks for defining variables which would look something like the following:

@let {
  one = 1;
  two = 2;
  sum = one + two;
}

This syntax isn't suitable, because:

  1. Developers used to writing TypeScript won't be familiar with it.
  2. It requires more keystrokes compared to the other proposals.
  3. We would have to define a new microsyntax within the block.
  4. Syntax highlighting will be trickier to implement.

Relates to #15280

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler target: rc This PR is targeted for the next release-candidate labels May 17, 2024
@ngbot ngbot bot modified the milestone: Backlog May 17, 2024
@crisbeto crisbeto marked this pull request as ready for review May 17, 2024 08:17
@NateRadebaugh
Copy link
Contributor

Not sure if there's an RFC for this but is there a performance impact to recomputing the value each time? Is there some automatic marshaling as signal that could memo-ize the value and avoid recomputing on subsequent re-renders if the parts of the expression do not change?

@robertIsaac
Copy link
Contributor

robertIsaac commented May 17, 2024

IMO it's better not to support multiple variables at the same line, I see in almost every project lint rules to error it in javascript/typescript files because it's confusing and very unclear
it took me few seconds to understand this statement and that that greeting is actually another variable
@let user = user$ | async, greeting = user ? 'Hello, ' + user.name : 'Loading';

so from the begging not supporting it would save everyone's time

@crisbeto
Copy link
Member Author

To elaborate on the decision to support multiple variables in a single declaration:

  1. It's meant to mimic variable declarations in JavaScript. We've seen multiple times over the years where deviating from the JS syntax ends up causing more confusion than it saves.
  2. This is fairly easy to lint against for projects that don't want it.
  3. The readability should be better once we have syntax highlighting for it.

@robertIsaac
Copy link
Contributor

robertIsaac commented May 19, 2024

  1. It's meant to mimic variable declarations in JavaScript. We've seen multiple times over the years where deviating from the JS syntax ends up causing more confusion than it saves.

I respectfully disagree here, it's not deviating from the JS syntax but following the best practice as per google style guide

Every local variable declaration declares only one variable: declarations such as let a = 1, b = 2; are not used.

and since we still have the exact one, just removing the ugly part of it, it won't be confusing for anyone
and in JS it's actually useful since compilers can use it to save some bytes from the production code, but in Angular template it doesn't make sense

  1. This is fairly easy to lint against for projects that don't want it.

still not implementing it will save everyone's time
Angular team needs to develop and keep mainiting it, and angular-eslint team needs to develop lint rules and auto fix for it and keep maintaining it

@crisbeto
Copy link
Member Author

We talked through it with the team and decided to drop the comma-separated syntax for now.

@AndrewKushnir AndrewKushnir removed the target: rc This PR is targeted for the next release-candidate label May 22, 2024
@AndrewKushnir
Copy link
Contributor

@crisbeto FYI, I've removed the "target" label for now - it was an "rc" and I was not sure if it should be "patch" or "minor" instead.

@crisbeto crisbeto added the target: patch This PR is targeted for the next patch release label May 23, 2024
@crisbeto
Copy link
Member Author

I'm setting it to patch since it's behind a flag.

@pumano
Copy link

pumano commented May 24, 2024

Very useful feature! Thank you!

Updates the lexer to produce tokens for `let` declarations. Currently it' behind a flag while the feature is being worked on.
Adds a new `LetDeclaration` node to the AST that captures the `LetStart`, `LetValue` and `LetEnd` tokens into a single node.
Introduces a new `LetDeclaration` into the Render3 AST, simiarly to the HTML AST, and adds an initial integration into the various visitors.
public name: string,
public value: string,
public sourceSpan: ParseSourceSpan,
readonly nameSpan: ParseSourceSpan,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought: I wonder if we should have this be the "keyspan" to match what we have in other places for the key/value separation.

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 was basing this on attributes that have a "name span" and a "value span".

while (this._cursor.peek() !== chars.$EOF) {
const char = this._cursor.peek();

// `@let` declarations terminate with a semicolon.
Copy link
Contributor

Choose a reason for hiding this comment

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

We continue consuming everything in the let declaration until we find a semicolon. I wonder if this is going to be annoying/problematic with the language service while you're actively typing out your variable declaration. The error span is going to reach the end of the file or until the first semicolon is found, right? I wonder if we can/should take some inspiration from some of the other "incomplete X" parsing (ex: 6ae3b68).

Not a blocker for this but probably something to adjust in the future when we're playing with it in the language service.

The way that we parse certain things in the compiler differs from TS/JS in some ways that can be problematic (ie for syntax highlighting - angular/vscode-ng-language-service#1998).

We might consider only continuing the parsing to the next line if the previous ends with a + (we don't support template strings AFAIK). There are probably other cases that I'm forgetting...

const a = 1
2
3;

TS parses the const as just a = 1 there but does 1+2 with this

const a = 1 + 
2
3;

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 think that's valid, but it would be tricky to know when to continue parsing versus when to give up. E.g. we'd also have to continue for things like && and ||. That's difficult to do at the moment, because the markup lexer and expression lexer don't know about each other.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: fw-i18n

@crisbeto crisbeto removed the action: review The PR is still awaiting reviews from at least one requested reviewer label May 29, 2024
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label May 29, 2024
@crisbeto crisbeto removed the request for review from alxhub May 29, 2024 19:06
thePunderWoman pushed a commit that referenced this pull request May 30, 2024
Adds a new `LetDeclaration` node to the AST that captures the `LetStart`, `LetValue` and `LetEnd` tokens into a single node.

PR Close #55848
thePunderWoman pushed a commit that referenced this pull request May 30, 2024
Introduces a new `LetDeclaration` into the Render3 AST, simiarly to the HTML AST, and adds an initial integration into the various visitors.

PR Close #55848
thePunderWoman pushed a commit that referenced this pull request May 30, 2024
Updates the lexer to produce tokens for `let` declarations. Currently it' behind a flag while the feature is being worked on.

PR Close #55848
thePunderWoman pushed a commit that referenced this pull request May 30, 2024
Adds a new `LetDeclaration` node to the AST that captures the `LetStart`, `LetValue` and `LetEnd` tokens into a single node.

PR Close #55848
thePunderWoman pushed a commit that referenced this pull request May 30, 2024
Introduces a new `LetDeclaration` into the Render3 AST, simiarly to the HTML AST, and adds an initial integration into the various visitors.

PR Close #55848
@thePunderWoman
Copy link
Contributor

This PR was merged into the repository by commit 9eef041.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants