Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

fix(compiler): handle ambient types in input transform function#51474

Closed
crisbeto wants to merge 4 commits intoangular:mainangular/angular:mainfrom
crisbeto:51424/input-transform-ambientcrisbeto/angular:51424/input-transform-ambientCopy head branch name to clipboard
Closed

fix(compiler): handle ambient types in input transform function#51474
crisbeto wants to merge 4 commits intoangular:mainangular/angular:mainfrom
crisbeto:51424/input-transform-ambientcrisbeto/angular:51424/input-transform-ambientCopy head branch name to clipboard

Conversation

@crisbeto
Copy link
Member

Fixes that the compiler was throwing an error if an ambient type is used inside of an input transform function. The problem was that the reference emitter was trying to write a reference to the ambient type's source file which isn't necessary.

Fixes #51424.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release area: compiler Issues related to `ngc`, Angular's template compiler labels Aug 23, 2023
@ngbot ngbot bot added this to the Backlog milestone Aug 23, 2023
@crisbeto crisbeto force-pushed the 51424/input-transform-ambient branch 3 times, most recently from 7d90aaa to bc1869f Compare August 24, 2023 04:17
@crisbeto crisbeto requested a review from alxhub August 24, 2023 04:59
@crisbeto crisbeto marked this pull request as ready for review August 24, 2023 04:59
Copy link
Member Author

Choose a reason for hiding this comment

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

Context here: in #43511 we added some assertions around not allowing classes defined inside namespaces. I added this flag so we only allow ambient references in specific places (input transforms in this case).

Copy link
Member

Choose a reason for hiding this comment

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

I can't think of a reason why we wouldn't want this behavior if AllowTypeImports is used; can the new condition just use the AllowTypeImports flag instead of introducing a new mode?

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like using AllowTypeImports also allows the behavior we were guarding against in #43511.

Copy link
Member

Choose a reason for hiding this comment

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

That is somewhat unexpected to me 🤔

packages/compiler-cli/src/ngtsc/reflection/src/host.ts Outdated Show resolved Hide resolved
Copy link
Member

@JoostK JoostK Aug 24, 2023

Choose a reason for hiding this comment

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

Unrelated to these changes, but this code explains #51190 (comment); the logic here should probably be updated to adopt the TypeEmitter/TypeParameterEmitter since it has some more logic to properly transplant literal types, which is currently broken for input transform transplantations.

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 we should tackle #51190 (comment) separately though. I'll add it to my list.

Copy link
Member

Choose a reason for hiding this comment

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

I can't think of a reason why we wouldn't want this behavior if AllowTypeImports is used; can the new condition just use the AllowTypeImports flag instead of introducing a new mode?

@crisbeto
Copy link
Member Author

@JoostK I got some time to revisit this PR. I've pushed the changes from #51474 (comment) in the fixup commit although I'm not exactly sure if this is what you meant.

@crisbeto crisbeto force-pushed the 51424/input-transform-ambient branch from 27441b6 to f791469 Compare November 17, 2023 14:33
packages/compiler-cli/src/ngtsc/imports/src/emitter.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/imports/src/references.ts Outdated Show resolved Hide resolved
@crisbeto crisbeto force-pushed the 51424/input-transform-ambient branch from f791469 to f577189 Compare December 4, 2023 11:57
@crisbeto
Copy link
Member Author

crisbeto commented Dec 4, 2023

@JoostK I've addressed 2/3 latest comments and responded on the other one.

Fixes that the compiler was throwing an error if an ambient type is used inside of an input `transform` function. The problem was that the reference emitter was trying to write a reference to the ambient type's source file which isn't necessary.

Fixes angular#51424.
@crisbeto crisbeto force-pushed the 51424/input-transform-ambient branch from f577189 to b932505 Compare December 13, 2023 09:48
@crisbeto
Copy link
Member Author

Addressed the last comment. The changes are in the final fixup commit.

@crisbeto crisbeto force-pushed the 51424/input-transform-ambient branch from b932505 to 83b6348 Compare December 13, 2023 10:53
@crisbeto crisbeto removed the request for review from alxhub December 13, 2023 10:53
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 13, 2023
@alxhub
Copy link
Member

alxhub commented Dec 13, 2023

This PR was merged into the repository by commit b98d8f7.

@alxhub alxhub closed this in b98d8f7 Dec 13, 2023
alxhub pushed a commit that referenced this pull request Dec 13, 2023
Fixes that the compiler was throwing an error if an ambient type is used inside of an input `transform` function. The problem was that the reference emitter was trying to write a reference to the ambient type's source file which isn't necessary.

Fixes #51424.

PR Close #51474
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 13, 2024
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…lar#51474)

Fixes that the compiler was throwing an error if an ambient type is used inside of an input `transform` function. The problem was that the reference emitter was trying to write a reference to the ambient type's source file which isn't necessary.

Fixes angular#51424.

PR Close angular#51474
rlmestre pushed a commit to rlmestre/angular that referenced this pull request Jan 26, 2024
…lar#51474)

Fixes that the compiler was throwing an error if an ambient type is used inside of an input `transform` function. The problem was that the reference emitter was trying to write a reference to the ambient type's source file which isn't necessary.

Fixes angular#51424.

PR Close angular#51474
amilamen pushed a commit to amilamen/angular that referenced this pull request Jan 26, 2024
…lar#51474)

Fixes that the compiler was throwing an error if an ambient type is used inside of an input `transform` function. The problem was that the reference emitter was trying to write a reference to the ambient type's source file which isn't necessary.

Fixes angular#51424.

PR Close angular#51474
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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.

Directive compile broken when using @Input transform

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.