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.

Sign up for GitHub

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

Jump to bottom

fix(ivy): ngtsc program emit ignoring custom transformers #27837

Closed
wants to merge 1 commit into from
Closed

fix(ivy): ngtsc program emit ignoring custom transformers #27837

wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Dec 26, 2018 •
edited

Fixes the customTransformers that are passed to the NgtscProgram.emit not being passed along to the emit callback.

@crisbeto crisbeto added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer comp: ivy labels Dec 26, 2018
@crisbeto crisbeto requested a review from alxhub December 26, 2018 15:18
@ngbot ngbot bot added this to the needsTriage milestone Dec 26, 2018
@mary-poppins
Copy link

You can preview 1877bd5 at https://pr27837-1877bd5.ngbuilds.io/.

alan-agius4
alan-agius4 reviewed Dec 26, 2018
View reviewed changes
packages/compiler-cli/src/ngtsc/program.ts Outdated
[ivyTransformFactory(this.compilation !, this.reflector, this.coreImportsFrom)];
const afterTransforms = customTransforms && customTransforms.afterTs || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I don’t think that || [] is required here as you should be able to pass undefined to emitCallback

alan-agius4
alan-agius4 approved these changes Dec 26, 2018
View reviewed changes
Copy link
Contributor

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

Thanks @crisbeto. LGTM.

@mary-poppins
Copy link

You can preview 5edba41 at https://pr27837-5edba41.ngbuilds.io/.

petebacondarwin
petebacondarwin requested changes Dec 28, 2018
View reviewed changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Any way to test this?

@crisbeto
Copy link
Member Author

I couldn't find any tests for reference @petebacondarwin. The NgtscProgram is a wrapper around TypeScript's Program so we'd essentially be testing that and whether we forward all the options to it.

mhevery
mhevery reviewed Dec 28, 2018
View reviewed changes
Copy link
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

All of the ones should have tests.

@mhevery
Copy link
Contributor

mhevery commented Jan 2, 2019

@alxhub can you suggest how to write some tests for this?

@crisbeto
fix(ivy): ngtsc program emit ignoring custom transformers
1bee7c4 
Fixes the `customTransformers` that are passed to the `NgtscProgram.emit` not being passed along.
@mary-poppins
Copy link

You can preview 1bee7c4 at https://pr27837-1bee7c4.ngbuilds.io/.

@crisbeto
Copy link
Member Author

crisbeto commented Jan 3, 2019

@petebacondarwin @mhevery I've added a unit test. Can you take another look?

petebacondarwin
petebacondarwin approved these changes Jan 3, 2019
View reviewed changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

This test works for me.
BTW normally I would have added this test as a fixup commit to the PR, to make it easier to review. I appreciate that it doesn't make much difference in this case. But I wanted to check that you are onboard with that approach of adding to PRs after review.

@crisbeto
Copy link
Member Author

crisbeto commented Jan 3, 2019

Of course, I wasn't aware that it was the convention. On the angular/material2 repo we usually keep squashing it into the initial commit.

@petebacondarwin
Copy link
Member

No problem. Fixup commits work well in this repo because the caretaker script will automatically squash them at merge time.

@mhevery mhevery 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 Jan 3, 2019
@mhevery
Copy link
Contributor

mhevery commented Jan 3, 2019

http://test/OCL:227698198:BASE:227698433:1546535490648:18706449

@kara kara added the target: major This PR is targeted for the next major release label Jan 4, 2019
@kara kara closed this in 13d23f3 Jan 4, 2019
devversion pushed a commit to devversion/angular that referenced this pull request Jan 9, 2019
@crisbeto @devversion
fix(ivy): ngtsc program emit ignoring custom transformers (angular#27837
6c6e05d 
)

Fixes the `customTransformers` that are passed to the `NgtscProgram.emit` not being passed along.

PR Close angular#27837
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
@crisbeto @ngfelixl
fix(ivy): ngtsc program emit ignoring custom transformers (angular#27837
eaab2c5 
)

Fixes the `customTransformers` that are passed to the `NgtscProgram.emit` not being passed along.

PR Close angular#27837
@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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@mhevery mhevery mhevery approved these changes

@alan-agius4 alan-agius4 alan-agius4 approved these changes

@alxhub alxhub Awaiting requested review from alxhub

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@crisbeto @mary-poppins @mhevery @petebacondarwin @alan-agius4 @kara @googlebot