Skip to content

Signal migration follow-ups#59463

Closed
devversion wants to merge 4 commits intoangular:mainfrom
devversion:signal-input-migration-followups
Closed

Signal migration follow-ups#59463
devversion wants to merge 4 commits intoangular:mainfrom
devversion:signal-input-migration-followups

Conversation

@devversion
Copy link
Copy Markdown
Member

@devversion devversion commented Jan 10, 2025

See individual commits

@devversion devversion 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 labels Jan 10, 2025
@devversion devversion requested a review from crisbeto January 10, 2025 13:12
@devversion devversion changed the title Signal input migration followups Signal input migration follow-ups Jan 10, 2025
crisbeto
crisbeto reviewed Jan 10, 2025
View reviewed changes
packages/core/schematics/migrations/signal-migration/test/ts-versions/index.bzl Outdated
Copy link
Copy Markdown
Member

@crisbeto crisbeto Jan 10, 2025

Choose a reason for hiding this comment

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

Should we just do one version per minor? e.g. 5.5, 5.6 and 5.7? AFAIK there shouldn't be huge behavior differences between them.

Copy link
Copy Markdown
Member Author

@devversion devversion Jan 10, 2025

Choose a reason for hiding this comment

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

I think you're right, but I also don't think it hurts having this increased coverage. WDYT?

Copy link
Copy Markdown
Member

@crisbeto crisbeto Jan 10, 2025

Choose a reason for hiding this comment

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

I don't feel strongly about it, but it'll slow down the CI a bit and might make it more noisy 🤷‍♂️

Copy link
Copy Markdown
Member Author

@devversion devversion Jan 10, 2025

Choose a reason for hiding this comment

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

It shouldn't run very often, but yeah. Let's just keep an eye on it and we can reduce soon anyway IIUC

@devversion
test: improve typescript version coverage for signal input migration
1c2b567 
This ensures the migration works for these TypeScript versions. The
migration is very sensitive to the TS version and its internals; so it
makes sense to test all of these.
@devversion
test: add test to verify extending tsconfig works in signal input mig…
bcbee89 
…ration

Related to angular#59348
@devversion
fixup! test: add test to verify extending tsconfig works in signal in…
c1762f0 
…put migration

ds
@devversion
fix(migrations): incorrect stats when migrating queries with best eff…
a2b0db0 
…ort mode

We previously did count forcibly ignored queries as incompatible. This
resulted in incorrect migration stats that are printed upon migration
completion.

See: angular#58657
@devversion devversion force-pushed the signal-input-migration-followups branch from 1239ec1 to a2b0db0 Compare January 10, 2025 14:19
@angular-robot angular-robot bot added the area: migrations Issues related to `ng update`/`ng generate` migrations label Jan 10, 2025
@ngbot ngbot bot added this to the Backlog milestone Jan 10, 2025
@devversion devversion changed the title Signal input migration follow-ups Signal migration follow-ups Jan 10, 2025
crisbeto
crisbeto approved these changes Jan 10, 2025
View reviewed changes
packages/core/schematics/migrations/signal-migration/test/ts-versions/index.bzl Outdated
Copy link
Copy Markdown
Member

@crisbeto crisbeto Jan 10, 2025

Choose a reason for hiding this comment

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

I don't feel strongly about it, but it'll slow down the CI a bit and might make it more noisy 🤷‍♂️

@devversion devversion 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 10, 2025
@kirjs
Copy link
Copy Markdown
Contributor

kirjs commented Jan 10, 2025

This PR was merged into the repository by commit 8496fb1.

The changes were merged into the following branches: main, 19.0.x, 19.1.x

kirjs pushed a commit that referenced this pull request Jan 10, 2025
@devversion @kirjs
test: improve typescript version coverage for signal input migration (#…
8820a6f 
…59463)

This ensures the migration works for these TypeScript versions. The
migration is very sensitive to the TS version and its internals; so it
makes sense to test all of these.

PR Close #59463
kirjs pushed a commit that referenced this pull request Jan 10, 2025
@devversion @kirjs
test: add test to verify extending tsconfig works in signal input mig…
09a9faf 
…ration (#59463)

Related to #59348

PR Close #59463
kirjs pushed a commit that referenced this pull request Jan 10, 2025
@devversion @kirjs
fix(migrations): incorrect stats when migrating queries with best eff…
eb2fcd1 
…ort mode (#59463)

We previously did count forcibly ignored queries as incompatible. This
resulted in incorrect migration stats that are printed upon migration
completion.

See: #58657

PR Close #59463
@kirjs kirjs closed this in 24f383a Jan 10, 2025
kirjs pushed a commit that referenced this pull request Jan 10, 2025
@devversion @kirjs
test: add test to verify extending tsconfig works in signal input mig…
86cbaaa 
…ration (#59463)

Related to #59348

PR Close #59463
kirjs pushed a commit that referenced this pull request Jan 10, 2025
@devversion @kirjs
fix(migrations): incorrect stats when migrating queries with best eff…
8496fb1 
…ort mode (#59463)

We previously did count forcibly ignored queries as incompatible. This
resulted in incorrect migration stats that are printed upon migration
completion.

See: #58657

PR Close #59463
kirjs pushed a commit that referenced this pull request Jan 10, 2025
@devversion @kirjs
test: improve typescript version coverage for signal input migration (#…
7debf0f 
…59463)

This ensures the migration works for these TypeScript versions. The
migration is very sensitive to the TS version and its internals; so it
makes sense to test all of these.

PR Close #59463
kirjs pushed a commit that referenced this pull request Jan 10, 2025
@devversion @kirjs
test: add test to verify extending tsconfig works in signal input mig…
ca1d392 
…ration (#59463)

Related to #59348

PR Close #59463
kirjs pushed a commit that referenced this pull request Jan 10, 2025
@devversion @kirjs
fix(migrations): incorrect stats when migrating queries with best eff…
59a1d68 
…ort mode (#59463)

We previously did count forcibly ignored queries as incompatible. This
resulted in incorrect migration stats that are printed upon migration
completion.

See: #58657

PR Close #59463
PrajaktaB27 pushed a commit to PrajaktaB27/angular that referenced this pull request Feb 7, 2025
@devversion @PrajaktaB27
test: improve typescript version coverage for signal input migration (a…
976b26d 
…ngular#59463)

This ensures the migration works for these TypeScript versions. The
migration is very sensitive to the TS version and its internals; so it
makes sense to test all of these.

PR Close angular#59463
PrajaktaB27 pushed a commit to PrajaktaB27/angular that referenced this pull request Feb 7, 2025
@devversion @PrajaktaB27
test: add test to verify extending tsconfig works in signal input mig…
9a367ea 
…ration (angular#59463)

Related to angular#59348

PR Close angular#59463
PrajaktaB27 pushed a commit to PrajaktaB27/angular that referenced this pull request Feb 7, 2025
@devversion @PrajaktaB27
fix(migrations): incorrect stats when migrating queries with best eff…
c70d741 
…ort mode (angular#59463)

We previously did count forcibly ignored queries as incompatible. This
resulted in incorrect migration stats that are printed upon migration
completion.

See: angular#58657

PR Close angular#59463
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Feb 10, 2025

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 Feb 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@crisbeto crisbeto crisbeto approved these changes

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: migrations Issues related to `ng update`/`ng generate` migrations target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

3 participants

@devversion @kirjs @crisbeto