feat: update to TS 2.9 and enable strictFunctionTypes#12182

Merged

jelbourn merged 1 commit into

angular:masterfrom

crisbeto:ts-2.9

Aug 28, 2018

Member

crisbeto commented Jul 12, 2018

Bumps the project to TypeScript 2.9, enables the strictFunctionTypes everywhere and fixes the various errors.

Fixes #12177.

crisbeto added the in progress label

crisbeto self-assigned this

crisbeto requested review from amcdnl, andrewseguin, devversion, jelbourn, josephperrott, mmalerba and tinayuangao as code owners

July 12, 2018 21:34

googlebot added the cla: yes label

crisbeto force-pushed the ts-2.9 branch 4 times, most recently from 3fe0cdb to 331feee Compare

July 14, 2018 13:12

amcdnl approved these changes

View reviewed changes

Contributor

amcdnl left a comment

LGTM

crisbeto force-pushed the ts-2.9 branch from 331feee to 0edfcf8 Compare

July 16, 2018 18:28

mmalerba approved these changes

View reviewed changes

src/cdk/layout/breakpoints-observer.ts

                     // have MediaQueryList inherit from EventTarget, which causes inconsistencies in how Zone.js
                     // patches it.
-                    (listener: MediaQueryListListener) => {
+                    (listener: Function) => {

Contributor

mmalerba Jul 16, 2018

Is there no type more specific than Function that we can use here?

Member Author

crisbeto Jul 16, 2018 •

edited

Loading

I don't think so. The strictFunctionTypes seem to resolve it to Function. A workaround is to have as MediaQueryListener before calling, but that seemed a little ugly.

Contributor

mmalerba Jul 16, 2018

Aright, sg

mmalerba added pr: lgtm action: merge labels

ngbot Bot commented Jul 16, 2018

I see that you just added the pr: merge ready label, but the following checks are still failing:
missing required label: "target: *"
status "ci/circleci: build" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

josephperrott removed the action: merge label

Member

josephperrott commented Jul 16, 2018

@crisbeto Is this still a WIP? If not go ahead and update the title and mark it merge ready again.

Member Author

crisbeto commented Jul 16, 2018

It is still is, I haven't been able to track down the Bazel issue yet.

crisbeto force-pushed the ts-2.9 branch 3 times, most recently from e467cde to 6fd2afb Compare

July 17, 2018 19:17

Member Author

crisbeto commented Jul 18, 2018

Looks like we're blocked here until angular/angular#24521 is resolved. cc @jelbourn

ngbot Bot commented Jul 18, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

crisbeto force-pushed the ts-2.9 branch from 6fd2afb to 0daee5c Compare

July 19, 2018 19:30

ngbot Bot commented Jul 24, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

crisbeto force-pushed the ts-2.9 branch from 5f2c741 to fc7e31e Compare

July 29, 2018 11:45

josephperrott removed request for josephperrott and tinayuangao

July 31, 2018 15:54

crisbeto force-pushed the ts-2.9 branch from fc7e31e to 4c54683 Compare

August 5, 2018 09:54

ngbot Bot commented Aug 20, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

2 similar comments

ngbot Bot commented Aug 20, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot Bot commented Aug 20, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

crisbeto force-pushed the ts-2.9 branch from 4c54683 to c30b3c2 Compare

August 21, 2018 16:39

ngbot Bot commented Aug 21, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

1 similar comment

ngbot Bot commented Aug 21, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

crisbeto force-pushed the ts-2.9 branch from c30b3c2 to fb4d3b3 Compare

August 27, 2018 17:21

crisbeto added action: merge target: minor and removed in progress labels

crisbeto changed the title ~~wip: update to TS 2.9 and enable strictFunctionTypes~~ feat: update to TS 2.9 and enable strictFunctionTypes

Contributor

jelbourn commented Aug 27, 2018 •

edited

Loading

@crisbeto getting this:

src/cdk/scrolling/virtual-scroll-viewport.ts:146:15 - error TS2345: Argument of type 'MonoTypeOperatorFunction<void>' is not assignable to parameter of type 'OperatorFunction<Event, void>'.
  Types of parameters 'source' and 'source' are incompatible.
    Type 'Observable<Event>' is not assignable to type 'Observable<void>'.
      Type 'Event' is not assignable to type 'void'.

146               startWith<void>(null!),

Probably new code introduced since your original change?

devversion reviewed

View reviewed changes

tools/dgeni/common/sort-members.ts

-              /** Combined type for a categorized method member document. */
-              type CategorizedMemberDoc = CategorizedMethodMemberDoc & CategorizedPropertyMemberDoc;
+              /** Sorts method members by deprecated status, member decorator, and name. */
+              export function sortCategorizedMethodMembers(docA: CategorizedMethodMemberDoc,

Member

devversion Aug 27, 2018 •

edited

Loading

Why do we need to duplicate the logic that actually is the same for properties and methods?

Did you run into any issue with the types that we should revisit later?

Member Author

crisbeto Aug 28, 2018

Yeah, it's something that started throwing after I enabled the strictFunctionTypes.

Contributor

jelbourn commented Aug 27, 2018

@crisbeto I looked at that error w/ Miles and it goes away if we change that line to:

startWith<Event | null>(null),

crisbeto force-pushed the ts-2.9 branch from fb4d3b3 to a2901c2 Compare

August 28, 2018 04:22

Member Author

crisbeto commented Aug 28, 2018

That's odd @jelbourn. I had fixed that error yesterday and I don't get any diff changes or compilation errors locally. Re-pushing to see if that fixes it.

Contributor

jelbourn commented Aug 28, 2018

Still failing on that line- it must be something different about Google's internal TS setup


          build: update to TS 2.9 and enable strictFunctionTypes

d566067

Bumps the project to TypeScript 2.9, enables the `strictFunctionTypes` everywhere and fixes the various errors.

Fixes angular#12177.

crisbeto force-pushed the ts-2.9 branch from a2901c2 to d566067 Compare

August 28, 2018 04:59

Member Author

crisbeto commented Aug 28, 2018

Could be. I've changed it to Event | null.

jelbourn merged commit d424c70 into angular:master

crisbeto added a commit to crisbeto/material2 that referenced this pull request


          build: clean up build workaround for ts 2.9

83ef901

Removes a workaround that was initially introduced to get angular#12182 through. The root issue has been resolved in angular#12876.

crisbeto mentioned this pull request

build: clean up build workaround for ts 2.9 #12912

Merged

jelbourn pushed a commit that referenced this pull request


          build: clean up build workaround for ts 2.9 (#12912)

b2ae3f4

Removes a workaround that was initially introduced to get #12182 through. The root issue has been resolved in #12876.

angular-automatic-lock-bot Bot commented Sep 9, 2019

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 Bot locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

devversion devversion left review comments

andrewseguin Awaiting requested review from andrewseguin

jelbourn Awaiting requested review from jelbourn

+2 more reviewers

mmalerba mmalerba approved these changes

amcdnl amcdnl approved these changes

Labels

action: merge cla: yes target: minor