Conversation
bea8850 to
091beed
Compare
JoostK
left a comment
There was a problem hiding this comment.
I am not sure if the changes to LView are necessary/desirable, but left some comments for your consideration.
There was a problem hiding this comment.
Can you expand on the motivation for these changes, e.g. in the commit message? It's unclear why this has been changed and it has quite a knock-on effect throughout the codebase.
There was a problem hiding this comment.
It's because LView[CONTEXT] can have 4 different types which made it difficult to deal with when combined with some of the new type checking changes. This saves us some as unknown as <something> casts across the codebase.
packages/core/src/render3/state.ts
Outdated
There was a problem hiding this comment.
It's generally a bad idea to have a generic type in an output position only, as it is guaranteed to be unsafe (i.e. the caller can specify any type argument without type-checking against some concrete value). I realise this isn't the first usage of such pattern (there's lots actually!) but it makes the reason why LView has a generic type less effective IMHO.
atscott
left a comment
There was a problem hiding this comment.
reviewed-for: language-service
|
I've addressed the feedback and expanded the PR description with more info on the changes. |
josephperrott
left a comment
There was a problem hiding this comment.
LGTM overall though I share some of @JoostK's concerns about the unsafe usage of generics with LView.
An additional question is if we should be landing this with ts as beta or wait until its at least in rc.
|
Due to the feature freeze, we have to land it as a beta and then update to RC during our RC period. The final version should be out before the v14 final release. |
There was a problem hiding this comment.
I'd propose adding more descriptive generic name:
| export interface LView<T = unknown> extends Array<any> { | |
| export interface LView<Context = unknown> extends Array<any> { |
There was a problem hiding this comment.
I don't mind making the change, but it goes against the convention we have everywhere else where we use single-character names for generics.
There was a problem hiding this comment.
I personally feel clarity is better than convention in this case.
JoostK
left a comment
There was a problem hiding this comment.
I continue to feel uncomfortable with the generic type arg for LView, but I haven't seen what the alternative would look like so can't compare which is better/worse.
|
I've addressed the latest feedback. Regarding making |
AndrewKushnir
left a comment
There was a problem hiding this comment.
Reviewed-for: public-api
jessicajaniuk
left a comment
There was a problem hiding this comment.
reviewed-for: public-api
There was a problem hiding this comment.
I personally feel clarity is better than convention in this case.
Adds support for TypeScript 4.7. Changes include: * Bumping the TS version as well as some Bazel dependencies to include bazel-contrib/rules_nodejs#3420. * Adding a backwards-compatibility layer for calls to `updateTypeParameterDeclaration`. * Making `LView` generic in order to make it easier to type the context based on the usage. Currently the context can be 4 different types which coupled with stricter type checking would required a lot of extra casting all over `core`. * Fixing a bunch of miscellaneous type errors. * Removing assertions of `ReferenceEntry.isDefinition` in a few of the language service tests. The field isn't returned by TS anymore and we weren't using it for anything. * Resolving in error in the language service that was caused by TS attempting to parse HTML files when we try to open them. Previous TS was silently setting them as `ScriptKind.Unknown` and ignoring the errors, but now it throws. I've worked around it by setting them as `ScriptKind.JSX`.
|
I've pushed an extra integration test which tries to import all our types using the new module resolution option. |
jelbourn
left a comment
There was a problem hiding this comment.
LGTM
Reviewed-for: public-api
|
This PR was merged into the repository by commit 29039fc. |
- [Angular supports TypeScript 4.7 until v14](angular/angular#45749)
|
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. |
Adds support for TypeScript 4.7. Changes include:
updateTypeParameterDeclaration.LViewgeneric in order to make it easier to type the context based on the usage. Currently the context can be 4 different types which coupled with stricter type checking would've required a lot of extra casting all overcore.ReferenceEntry.isDefinitionin a few of the language service tests. The field isn't returned by TS anymore and we weren't using it for anything.ScriptKind.Unknownand ignoring the errors, but now it throws. I've worked around it by setting them asScriptKind.JSX.