fix(core): update unknown tag error for jit standalone components#45920
fix(core): update unknown tag error for jit standalone components#45920dario-piotrowicz wants to merge 7 commits intoangular:mainfrom
Conversation
|
I wanted to add unit tests for this, but it does not seem like there are any unit tests regarding this sort of functionality, please let me know if I should add some somewhere (I may be wrong but I imagine that jit is almost deprecated in favour of aot? so maybe that's why unit testing functionalities here is not as important?) |
|
@dario-piotrowicz thanks for creating this PR! I think we should try to use the approach proposed by Joost (in #45818 (comment)) vs adding a flag to the Quick comment: let's update the commit message to say |
|
@dario-piotrowicz yes, I still believe that we should add relevant tests for the JIT case to exercise changes to packages/core/src/render3/instructions/element.ts. As for the test location I've suggested on in my previous comment #45818 (comment) |
|
@AndrewKushnir @pkozlowski-opensource Thank you very much 🙂 I'll address all your comments this coming weekend, sorry that it's been a slow few days 😓 |
75be8dc to
e1e19a3
Compare
6687d29 to
61fe62a
Compare
There was a problem hiding this comment.
I feel like we may need this logic in other error messages as we start to update them with standalone context more. I'd like to propose that we extract it to a separate function (for ex. isHostComponentStandalone and put it in render3/instructions/shared.ts file) and add a check like this:
angular/packages/core/src/render3/state.ts
Line 355 in ca04ece
It should help avoid adding this relatively slow path into the perf-sensitive code (like change detection). If we come across a situation when we need similar detection in the non-dev mode, we'd need to re-consider adding a flag to the
TView (as it was implemented originally in this PR).
There was a problem hiding this comment.
sounds good! 🙂👍
I actually did think of creating a function for this piece of code but I was afraid of overengineer things 😅
There was a problem hiding this comment.
Agree with @AndrewKushnir - we should have a utility function here.
There was a problem hiding this comment.
I've added the isHostComponentStandalone function please have a look 🙂
(by the way, seems like the other functions in the shared file don't have unit tests associated to them, what do you think should I add one for the new function? if so could you suggest a file? 🙏)
Also, would it be worth it to also extract this two lines into another function?
const declarationLView = lView[DECLARATION_COMPONENT_VIEW] as LView<Type<unknown>>;
const context = declarationLView[CONTEXT];(something like getLViewContext or something along those lines), let me know if this is a good idea and in that case if you'd like me to do it here or in a separate PR 🙂
There was a problem hiding this comment.
@AndrewKushnir , @pkozlowski-opensource any comment on this one? 🙂
61fe62a to
5804f98
Compare
add missing semicolon
add no context check
…ents update the error message presented during aot compilation when an unrecognized tag/element is found in a standalone component so that it does not mention the ngModule anymore Note: the jit variant is present in PR angular#45920 resolves angular#45818
|
This PR was merged into the repository by commit b1a4b30. |
|
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. |
update the error message presented during jit compilation when an unrecognized
tag/element is found in a standalone component so that it does not mention
the ngModule anymore
Note: the aot variant is present in PR #45919
resolves #45818
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Issue
Issue Number: #45818
Does this PR introduce a breaking change?
Other information
Result:
