Template type checking support for @let#56199
Closed
crisbeto wants to merge 8 commits intoangular:mainfrom
Closed
Conversation
crisbeto
commented
May 31, 2024
Member
Author
There was a problem hiding this comment.
Context for this type is that we were repeating Reference | Variable in a bunch of places which would now have to include LetDeclaration. I moved it out to a type to make it easier to manage.
Adds a private `_enableLetSyntax` flag that allows for let declarations to be enabled in tests.
JoostK
reviewed
Jun 3, 2024
Member
JoostK
left a comment
There was a problem hiding this comment.
I haven't seen any diagnostics for duplicate @let declaration identifiers in the same scope.
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Outdated
Show resolved
Hide resolved
Comment on lines
2844
to
2853
Member
There was a problem hiding this comment.
Do we want to allow this? I feel like I'd rather not allow this, at least initially, unless there's some use-cases that benefit from this ability?
Member
Author
There was a problem hiding this comment.
I initially had it set up to flag cases like this as well, but I changed it after talking with @alxhub about it. My thinking is:
- It matches how local references work so it would be more familiar for users.
- There's no technical reason why we can't support it since the value is guaranteed to be defined before child views attempt to reference it. There's one edge case where they can read an un-initialized variable, but I have safeguards against it in a future commit.
- At least to me, it makes sense to "hoist" it, if we treat embedded views as JS functions.
Also worth noting that @let will be in developer preview initially and I'll keep an eye on feedback around this
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Outdated
Show resolved
Hide resolved
Integrates the let declarations into the template binder which will be used to power the scoping behavior of the new syntax.
…type checker Integrates let declarations into the template type checker by producing corresponding constants in the TCB. This also includes a couple of custom diagnostics to flag usages of let before they're declared and illegal writes to let declarations. We can't rely on TS for these checks, because it includes the variable name in the diagnostic.
Adds support for let declarations in the template indexer.
Updates the symbol builder to handle resolving the symbol of a let declaration.
Adds support for let declarations inside the `CompletionEngine`.
…ng check Updates the check that prevent writes to template variables in two-way bindings to account for let declarations. Also fixes some old tests that weren't properly setting up two-way bindings.
Adds a template diagnostic that will flag cases where multiple `@let` declarations use the same name.
Member
Author
|
@JoostK I've addressed the feedback and added another commit for duplicate declarations. |
JoostK
approved these changes
Jun 4, 2024
atscott
approved these changes
Jun 4, 2024
thePunderWoman
approved these changes
Jun 4, 2024
Contributor
thePunderWoman
left a comment
There was a problem hiding this comment.
reviewed-for: public-api
Member
Author
|
Caretaker note: there's one pre-existing broken target that can be ignored. |
thePunderWoman
pushed a commit
that referenced
this pull request
Jun 4, 2024
…type checker (#56199) Integrates let declarations into the template type checker by producing corresponding constants in the TCB. This also includes a couple of custom diagnostics to flag usages of let before they're declared and illegal writes to let declarations. We can't rely on TS for these checks, because it includes the variable name in the diagnostic. PR Close #56199
Contributor
|
This PR was merged into the repository by commit 9aea8a0. |
thePunderWoman
pushed a commit
that referenced
this pull request
Jun 4, 2024
) Adds support for let declarations inside the `CompletionEngine`. PR Close #56199
thePunderWoman
pushed a commit
that referenced
this pull request
Jun 4, 2024
…type checker (#56199) Integrates let declarations into the template type checker by producing corresponding constants in the TCB. This also includes a couple of custom diagnostics to flag usages of let before they're declared and illegal writes to let declarations. We can't rely on TS for these checks, because it includes the variable name in the diagnostic. PR Close #56199
thePunderWoman
pushed a commit
that referenced
this pull request
Jun 4, 2024
) Adds support for let declarations inside the `CompletionEngine`. PR Close #56199
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These changes include the code necessary to type check
@letdeclarations. Also some prerequisites for the language service integration.refactor(compiler-cli): add compiler flag for testing let declarations
Adds a private
_enableLetSyntaxflag that allows for let declarations to be enabled in tests.refactor(compiler): integrate let declarations into the template binder
Integrates the let declarations into the template binder which will be used to power the scoping behavior of the new syntax.
refactor(compiler-cli): integrate let declarations into the template type checker
Integrates let declarations into the template type checker by producing corresponding constants in the TCB.
This also includes a couple of custom diagnostics to flag usages of let before they're declared and illegal writes to let declarations. We can't rely on TS for these checks, because it includes the variable name in the diagnostic.
refactor(compiler-cli): integrate let declaration into the indexer
Adds support for let declarations in the template indexer.
refactor(compiler-cli): support resolving the symbol of let declaration
Updates the symbol builder to handle resolving the symbol of a let declaration.
refactor(compiler-cli): support completions for let declarations
Adds support for let declarations inside the
CompletionEngine.refactor(compiler-cli): account for let declarations in two-way binding check
Updates the check that prevent writes to template variables in two-way bindings to account for let declarations.
Also fixes some old tests that weren't properly setting up two-way bindings.
refactor(compiler-cli): add diagnostic for duplicate let declarations
Adds a template diagnostic that will flag cases where multiple
@letdeclarations use the same name.