microsoft / fast

The adaptive interface system for modern web experiences.
https://www.fast.design
Other
9.28k stars 595 forks source link

[archive] Fix usage with TypeScript 4.8+ #6779

Closed fcollonval closed 1 year ago

fcollonval commented 1 year ago

Pull Request

šŸ“– Description

Add typing to template variable to remove error seen in code consuming fast-foundation with TypeScript 4.8+

šŸŽ« Issues

Fixes #6365

šŸ‘©ā€šŸ’» Reviewer Notes

šŸ“‘ Test Plan

āœ… Checklist

General

I'm not sure how to test for this. I force locally a TypeScript update to 4.9.5 and fixed the error mentioned in @microsoft/fast-foundation.

There are also error in @microsoft/fast-router but this does not fix them.

Errors in @microsoft/fast-router
src/configuration.ts(49,9): error TS2322: Type 'DefaultRouteRecognizer' is not assignable to type 'RouteRecognizer'.
  The types returned by 'recognize(...)' are incompatible between these types.
    Type 'Promise | null>' is not assignable to type 'Promise | null>'.
      Type 'RecognizedRoute | null' is not assignable to type 'RecognizedRoute | null'.
        Type 'RecognizedRoute' is not assignable to type 'RecognizedRoute'.
          Type 'unknown' is not assignable to type 'TSettings'.
            'TSettings' could be instantiated with an arbitrary type which could be unrelated to 'unknown'.
src/recognizer.ts(271,17): error TS18047: 'segmentA' is possibly 'null'.
src/recognizer.ts(271,33): error TS18047: 'segmentB' is possibly 'null'.
src/recognizer.ts(275,17): error TS18047: 'segmentA' is possibly 'null'.
src/recognizer.ts(275,33): error TS18047: 'segmentB' is possibly 'null'.
src/router.ts(201,13): error TS2322: Type 'unknown' is not assignable to type 'Router | null | undefined'.

ā­ Next Steps

chrisdholt commented 1 year ago

Thanks for this @fcollonval - do we have a repro of this I can check out? I recall us seeing a similar TS issue which disappeared with a different fix which wasn't specific to 4.8. If I can validate the issue, happy to take an update/fix assuming it makes sense.

fcollonval commented 1 year ago

Thanks @chrisdholt

We have seen that first when working on https://github.com/brichet/jupyterlab/tree/toolbar_webcomponent (uses TypeScript 5.0.4).

But I bumped TypeScript (to 4.9.5) in the components library we are using built on fast v1 to ease error reproducibility: https://github.com/jupyterlab-contrib/jupyter-ui-toolkit/actions/runs/5600572055/jobs/10243153138?pr=63#step:9:200

chrisdholt commented 1 year ago

Thanks @chrisdholt

We have seen that first when working on https://github.com/brichet/jupyterlab/tree/toolbar_webcomponent (uses TypeScript 5.0.4).

But I bumped TypeScript (to 4.9.5) in the components library we are using built on fast v1 to ease error reproducibility: https://github.com/jupyterlab-contrib/jupyter-ui-toolkit/actions/runs/5600572055/jobs/10243153138?pr=63#step:9:200

Thanks for this, sorry for the delay. I'm going to fork and pull your repo to try and reproduce this. I have a feeling it may not be the exact TS issue. I also could be wrong :)

fcollonval commented 1 year ago

Thanks a lot for taking the time @chrisdholt

m-akinc commented 1 year ago

Any updates? Also interested in using TS 4.8+.

rajsite commented 1 year ago

The TypeScript version upgrade is blocking us from performing Angular upgrades without having all our applications disable library type checking.

Angular 14 support ends in November and we would like to be able to migrate to Angular 16 which requires TypeScript >=4.9.3.

Ideally we can avoid falling out of supported Angular version usage so hopefully within the next week or two we would like FAST to be compatible with a newer TypeScript version so we can bubble up support through our libraries. @chrisdholt do you think that would be possible?

chrisdholt commented 1 year ago

The TypeScript version upgrade is blocking us from performing Angular upgrades without having all our applications disable library type checking.

Angular 14 support ends in November and we would like to be able to migrate to Angular 16 which requires TypeScript >=4.9.3.

Ideally we can avoid falling out of supported Angular version usage so hopefully within the next week or two we would like FAST to be compatible with a newer TypeScript version so we can bubble up support through our libraries. @chrisdholt do you think that would be possible?

We're focused on updating the latest alpha and beta versions, but it's requiring some changes to our testing libraries. The bulk of my effort for TS right now is going to focus on moving those versions forward towards stable and hopefully getting onto the latest TS version for alpha/beta versions so we can leverage TS's support for decorator metadata.

As it pertains to the archive branches - we can try to get some support there, but I would suggest starting to plan on how to move off those. Given the alpha/beta changes have been moving slower, we've continued to backport some fixes, but I don't see that going on forever. At some point, the code there is just difficult to maintain with backwards compatibility, especially given the amount of the time the FAST Components package has been deprecated. I'm keeping this open with the hopes we can get to updating the archive, but changes there are more difficult and time consuming as of now.

chrisdholt commented 1 year ago

The TypeScript version upgrade is blocking us from performing Angular upgrades without having all our applications disable library type checking. Angular 14 support ends in November and we would like to be able to migrate to Angular 16 which requires TypeScript >=4.9.3. Ideally we can avoid falling out of supported Angular version usage so hopefully within the next week or two we would like FAST to be compatible with a newer TypeScript version so we can bubble up support through our libraries. @chrisdholt do you think that would be possible?

We're focused on updating the latest alpha and beta versions, but it's requiring some changes to our testing libraries. The bulk of my effort for TS right now is going to focus on moving those versions forward towards stable and hopefully getting onto the latest TS version for alpha/beta versions so we can leverage TS's support for decorator metadata.

As it pertains to the archive branches - we can try to get some support there, but I would suggest starting to plan on how to move off those. Given the alpha/beta changes have been moving slower, we've continued to backport some fixes, but I don't see that going on forever. At some point, the code there is just difficult to maintain with backwards compatibility, especially given the amount of the time the FAST Components package has been deprecated. I'm keeping this open with the hopes we can get to updating the archive, but changes there are more difficult and time consuming as of now.

Good news - I repro'd this locally when trying to update the repo. Let's see if @nicholasrice has any concerns with this; he may have ideas on where/why the constraint may be failing in this scenario as well.

chrisdholt commented 1 year ago

Update on this - I got some insight from the TS team on why this is throwing now. I think we need to look at the best path forward and update this w/ TS.

chrisdholt commented 1 year ago

Made it through getting the repo building with TS 4.8+, unfortunately it requires changing out all the test suites due to ESM requirements. Building this locally and if it passes I plan to push it through. Thanks @fcollonval for this PR and all for your patience.

fcollonval commented 1 year ago

Thanks @chrisdholt for taking the time of looking at this maintenance PR.

rajsite commented 1 year ago

Made it through getting the repo building with TS 4.8+, unfortunately it requires changing out all the test suites due to ESM requirements. Building this locally and if it passes I plan to push it through. Thanks @fcollonval for this PR and all for your patience.

That's awesome news, thanks @chrisdholt! Do you know when we might see the changes in a published package?

rajsite commented 1 year ago

Made it through getting the repo building with TS 4.8+, unfortunately it requires changing out all the test suites due to ESM requirements. Building this locally and if it passes I plan to push it through. Thanks @fcollonval for this PR and all for your patience.

That's awesome news, thanks @chrisdholt! Do you know when we might see the changes in a published package?

These changes are now in @microsoft/fast-foundation@2.49.2 šŸŽ‰