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.

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

fix: disable injectable-pipe migration #30180

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@crisbeto
Copy link
Member

commented Apr 28, 2019

Disables the injectable pipe migration until we can decide whether this is the right solution for Ivy. Rolling it out properly will involve a more detailed plan and more changes like updating the styleguide, scaffolding schematics etc.

Context for the new test-migrations.json: since we use the migrations.json both for the real migrations and for tests, it doesn't allow us to disable a schematic while continuing to run its tests. This change adds the test-specific file so that we can continue running the injectable-pipe tests, even though the schematic itself is disabled.

@crisbeto crisbeto requested a review from angular/fw-core as a code owner Apr 28, 2019

@googlebot googlebot added the cla: yes label Apr 28, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

@crisbeto crisbeto force-pushed the crisbeto:injectable-pipe-disable branch 2 times, most recently from 43d0780 to 56aa0ef Apr 28, 2019

@IgorMinar
Copy link
Member

left a comment

Lgtm but can you please extend the commit message to contain more details. The problem is that we are not yet sure if this is the right solution for ivy and if it is we need to do much more to roll out this change: e.g. update style guide, scaffolding schematics, docs and tutorial, etc etc. So it's better to wait until we are sure that we want this change and then make a comprehensive plan on how to roll it out.

@IgorMinar

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

@crisbeto Thanks for putting this together so quickly.

fix: disable injectable-pipe migration
Disables the injectable pipe migration until we can decide whether this is the right solution for Ivy. Rolling it out properly will involve a more detailed plan and more changes like updating the styleguide, scaffolding schematics etc.

Context for the new `test-migrations.json`: since we use the `migrations.json` both for the real migrations and for tests, it doesn't allow us to disable a schematic, but continue running its tests. This change adds the test-specific file so that we can continue running the `injectable-pipe` tests, even though the schematic itself is disabled.

@crisbeto crisbeto force-pushed the crisbeto:injectable-pipe-disable branch from 56aa0ef to b183a44 Apr 28, 2019

@crisbeto

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2019

Updated the commit message and PR description with more context.

@devversion
Copy link
Member

left a comment

Ideally there would have been only a new collection for the injectable-pipe schematic in order to run tests without shipping it. Using the real migration collection allows us to validate that the schematics (which are intended to run publicly) are wired up correctly.

I don't mind too much though. LGTM 😄

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

Presubmit #2 (after additional changes)

@crisbeto

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

@devversion I can see it going either way. I did it this way, because it's more consistent.

@devversion

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@crisbeto True it's more consistent for the tests, but given that it means that we need to duplicate schematic collection entries and we no longer test the publicly enabled migrations with the actual ng-update collection, I think we should change it back and only have a collection for the injectable-pipe.

Doesn't have to be part of this PR as it already had various presubmits..

AndrewKushnir added a commit that referenced this pull request Apr 29, 2019

fix: disable injectable-pipe migration (#30180)
Disables the injectable pipe migration until we can decide whether this is the right solution for Ivy. Rolling it out properly will involve a more detailed plan and more changes like updating the styleguide, scaffolding schematics etc.

Context for the new `test-migrations.json`: since we use the `migrations.json` both for the real migrations and for tests, it doesn't allow us to disable a schematic, but continue running its tests. This change adds the test-specific file so that we can continue running the `injectable-pipe` tests, even though the schematic itself is disabled.

PR Close #30180

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

fix: disable injectable-pipe migration (angular#30180)
Disables the injectable pipe migration until we can decide whether this is the right solution for Ivy. Rolling it out properly will involve a more detailed plan and more changes like updating the styleguide, scaffolding schematics etc.

Context for the new `test-migrations.json`: since we use the `migrations.json` both for the real migrations and for tests, it doesn't allow us to disable a schematic, but continue running its tests. This change adds the test-specific file so that we can continue running the `injectable-pipe` tests, even though the schematic itself is disabled.

PR Close angular#30180
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.