Skip to content

refactor(ng-update): better check for updated constructor signatures - #12970

Merged
jelbourn merged 1 commit into
angular:masterfrom
devversion:refactor/ng-update-signature-check
Sep 5, 2018
Merged

refactor(ng-update): better check for updated constructor signatures#12970
jelbourn merged 1 commit into
angular:masterfrom
devversion:refactor/ng-update-signature-check

Conversation

@devversion

Copy link
Copy Markdown
Member

Instead of just checking the length of the constructor arguments, we now check the types of the constructor or super call. This means that we can way better report invalid signatures for constructor changes like for the MatCalendar (#9775). Just relying on the length of arguments means that order is being ignored.

This also makes maintaining the constructor signature changes easier (there are a lot of instances for V7).

NOTE: There is one downside: Right now since we cannot check whether a type is assignable to another one, things like that will result in a false report.

interfaceExtendedDocumentextendsDocument{}newMatIconRegistry({},{},{}asExtendedDocument);

But IMO since this just results in a polite message from TSLint, and we benefit a lot more for things like the MatCalendar, we should be fine having this trade-off. We can follow-up with some custom checks.

@devversiondevversion added pr: merge safe target: major This PR is targeted for the next major release labels Sep 4, 2018
@googlebotgooglebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 4, 2018
invalidArgCounts: [
{
count: 2,
message: '"g{{defaults}}" is now required as a third argument'

@devversiondevversionSep 4, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

FYI: There will be a follow-up PR that adds some additional messages to the TSLint failure.

Even though it's not necessarily needed because we show the new signature, it's good to explain people what changed.

@devversion

Copy link
Copy Markdown
MemberAuthor

Note: Technically the type checker wouldn't be a good idea to use for the schematics because when doing an upgrade from V5 to V7, we won't have typings for V5 and V6 once ng update runs.

But since the Angular CLI always installs the desired target version, we will have the typings for that version and can just check the signatures. Since we just check and not migrate anything, it's even better just displaying the signature of the target Material version.

e.g. consider the following signatures for each version:

  • V5: MatAutocomplete#constructor(String, String)
  • V6: MatAutocomplete#constructor(String, String, Number)
  • V7: MatAutocomplete#constructor(String, String)

In that case if someone upgrades from V5 to V7, we should not add a TSLint failure. But if someone upgrades from V5 to only V6, it will throw. Same if V6 to V7 runs.

@jelbournjelbourn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM


import {transformChanges} from '../transform-change-data';

export const constructorChecks = transformChanges<string>([

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add a comment here explaining what this config is for?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Can do. Will do that while rebasing.

@jelbournjelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Sep 4, 2018
@devversiondevversion added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Sep 4, 2018
@devversion
devversionforce-pushed the refactor/ng-update-signature-check branch from 487dca1 to 22e8f53CompareSeptember 4, 2018 20:19
Instead of just checking the length of the constructor arguments, we now check the types of the constructor or super call. This means that we can *way* better report invalid signatures for constructor changes like for the `MatCalendar` (angular#9775). Just relying on the length of arguments means that *order* is being ignored.
This also makes maintaining the constructor signature changes easier (there are a lot of instances for V7).
@devversion
devversionforce-pushed the refactor/ng-update-signature-check branch from 22e8f53 to 18c91daCompareSeptember 4, 2018 20:20
@devversiondevversion added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Sep 4, 2018
@devversion

Copy link
Copy Markdown
MemberAuthor

@jelbourn Rebased.

@jelbourn
jelbourn merged commit e124418 into angular:masterSep 5, 2018
@angular-automatic-lock-bot

Copy link
Copy Markdown

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-botangular-automatic-lock-botBot locked and limited conversation to collaborators Sep 9, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: mergeThe PR is ready for merge by the caretakercla: yesPR author has agreed to Google's Contributor License Agreementtarget: majorThis PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@devversion@jelbourn@googlebot