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.

Sign up for GitHub

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

Jump to bottom

chore: update typescript to 4.6.3 #5788

Merged
merged 6 commits into from
Apr 26, 2022
Merged

chore: update typescript to 4.6.3 #5788

merged 6 commits into from
Apr 26, 2022

Conversation

chrisdholt
Copy link
Member

@chrisdholt chrisdholt commented Mar 30, 2022 •
edited
Loading

Pull Request

📖 Description

closes #5198

👩‍💻 Reviewer Notes

  • issues with tooling on form association conflicts have been resolved. Current issue is now the need to ingest tooling as ESM but fast-components not being a "type": "module".
  • no duck typing for ARIAMixin conflicts. Proposed resolution here is that we add the overlap string | null. I still like being explicit with the values when applicable for scenarios where those are the only actual valid values.

Please chime in if you disagree with the approaches on the issues outlined above.

📑 Test Plan

✅ Checklist

General

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

⏭ Next Steps

@chrisdholt
Copy link
Member Author

chrisdholt commented Mar 30, 2022 •
edited
Loading

Looks like skipLibCheck is just covering up issues. We'll need to dig in to figure out a good solution here. nohoist was not working, likely due to these existing in symlinked packages within the repo - so all shared deps are hoisted.

Skip lib check issues have been resolved - updated issues noted above.

@chrisdholt chrisdholt changed the title chore: update typescript to 4.6.2 chore: update typescript to 4.6.3 Mar 31, 2022
@chrisdholt chrisdholt force-pushed the users/chhol/update-typescript-to-4.6.2 branch from 5c3435c to 0ee9a4d Compare March 31, 2022 03:29
radium-v
radium-v reviewed Apr 6, 2022
View reviewed changes
package.json
@@ -133,7 +134,7 @@
"node-notifier": "^9.0.0",
"trim-newlines": "^4.0.2",
"trim": "^0.0.3",
"typescript": "^3.9.0",
"typescript": "^4.6.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This resolution forces all typescript dependencies to match. Is it still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm attempting to push forward with parity in order to keep this change as minimally disruptive as possible. My concern is that we end up with unexpected outcomes by removing this in this PR. This and it's existence as a dependency is a solid question and deserving of it's own issue IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to take this in a separate PR, my assumption is that it is needed due to the breaking changes within minor versions. We'll need to investigate in full though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OH api-extractor uses its own version of Typescript without this. We don't want that.

package.json Show resolved Hide resolved
packages/utilities/fast-eslint-rules/package.json Show resolved Hide resolved
packages/web-components/fast-element/package.json
@@ -50,6 +50,7 @@
"@types/karma": "^5.0.0",
"@types/mocha": "^7.0.2",
"@types/webpack-env": "^1.15.2",
"@types/web-ie11": "^0.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we support IE11?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hard to add a reference in the package.json so I anticipated this :). The issue is specifically a break in types that creates an issue with document.createTreeWalker(). Updating beyond 4.3.5 creates this issue so this explicitly exists to support the typing. microsoft/TypeScript#33462

This will be removed in @microsoft/fast-element v2 as I believe the method is pulled from there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Affirmative. No more use of tree walker in 2.0.

@chrisdholt
Copy link
Member Author

PR is blocked by the es module update

@chrisdholt chrisdholt force-pushed the users/chhol/update-typescript-to-4.6.2 branch 2 times, most recently from 6d72da3 to 7aac7a3 Compare April 7, 2022 21:04
@chrisdholt chrisdholt marked this pull request as ready for review April 7, 2022 21:06
@chrisdholt chrisdholt requested a review from radium-v April 7, 2022 21:06
@chrisdholt chrisdholt force-pushed the users/chhol/update-typescript-to-4.6.2 branch from 7787eb0 to b836568 Compare April 7, 2022 22:05
@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-5788.centralus.azurestaticapps.net

@chrisdholt chrisdholt force-pushed the users/chhol/update-typescript-to-4.6.2 branch from 74102a3 to 7de16bc Compare April 20, 2022 00:07
@chrisdholt
Copy link
Member Author

chrisdholt commented Apr 20, 2022 •
edited
Loading

@nicholasrice I'm not able to repro this build failure locally...I have no idea why - when you get a second can you pull down and see if you can repro and if so, push a fix to my branch? No idea why I can't repro this anymore locally... 🤷‍♂️

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-5788.centralus.azurestaticapps.net

@nicholasrice
Copy link
Contributor

@nicholasrice I'm not able to repro this build failure locally...I have no idea why - when you get a second can you pull down and see if you can repro and if so, push a fix to my branch? No idea why I can't repro this anymore locally... 🤷‍♂️

Yeah I'll try to have a look today.

@radium-v radium-v mentioned this pull request Apr 21, 2022
Closed
@nicholasrice
Copy link
Contributor

@chrisdholt This also works fine on my machine. Perhaps it's some kind of dependency caching issue on the CI?

@chrisdholt
Copy link
Member Author

@chrisdholt This also works fine on my machine. Perhaps it's some kind of dependency caching issue on the CI?

I'm not crazy! Sounds like it could be - I'll investigate the pipelines a bit.

@chrisdholt chrisdholt force-pushed the users/chhol/update-typescript-to-4.6.2 branch 3 times, most recently from 9227b05 to 6417ba7 Compare April 23, 2022 00:30
@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://blue-coast-0df7a6610-5788.centralus.azurestaticapps.net

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-sand-03fe10e10-5788.centralus.azurestaticapps.net

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://blue-coast-0df7a6610-5788.centralus.azurestaticapps.net

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-5788.centralus.azurestaticapps.net

1 similar comment
@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-5788.centralus.azurestaticapps.net

@chrisdholt chrisdholt force-pushed the users/chhol/update-typescript-to-4.6.2 branch from ba456ac to b6bcb61 Compare April 26, 2022 19:49
@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-sand-03fe10e10-5788.centralus.azurestaticapps.net

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://blue-coast-0df7a6610-5788.centralus.azurestaticapps.net

@chrisdholt chrisdholt force-pushed the users/chhol/update-typescript-to-4.6.2 branch from 9c57f2b to b6bcb61 Compare April 26, 2022 20:06
@chrisdholt
Copy link
Member Author

After adding the Yarn Cache back on the build server, it seems clear that it is preventing us from passing the prepare step. I'm going to remove this and we can see about getting it back in retroactively, but currently it seems like a bug that we can't get past.

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-5788.centralus.azurestaticapps.net

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://blue-coast-0df7a6610-5788.centralus.azurestaticapps.net

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-sand-03fe10e10-5788.centralus.azurestaticapps.net

chrisdholt and others added 6 commits April 26, 2022 13:11
@chrisdholt
update to typescript 4.6.2 and update ARIAMixin typings
41adbc4 
use skip lib check to avoid conflicts with fast-tooling

Change files

update package.json resolution for types/react because docusaurus is causing an upgrade to v18.0

update readme and yarn.lock files

fix eslint errors in fast-element
@williamw2 @chrisdholt
chore: fix links due to ts upgrade (#5825)
36737b7 
# Pull Request

## 📖 Description

The Typescript version upgrade changes how it handles the @link JSDOC tag in comments causing them to be removed when the CEM Analyzer process the code. This results in the link URLs and text being rendered to markdown in such a way that they cannot be corrected into markdown links. This PR adds a CEM Analyzer plugin that checks for the existence of @link tags in the descriptions generated by "comment-parser" and retains the original text.

<!---
Provide some background and a description of your work.
What problem does this change solve?
Is this a breaking change, chore, fix, feature, etc?
-->

### 🎫 Issues

<!---
* List and link relevant issues here.
-->

## 👩‍💻 Reviewer Notes

<!---
Provide some notes for reviewers to help them provide targeted feedback and testing.

Do you recommend a smoke test for this PR? What steps should be followed?
Are there particular areas of the code the reviewer should focus on?
-->

## 📑 Test Plan

<!---
Please provide a summary of the tests affected by this work and any unique strategies employed in testing the features/fixes.
-->

## ✅ Checklist

### General

<!--- Review the list and put an x in the boxes that apply. -->

- [ ] I have included a change request file using `$ yarn change`
- [ ] I have added tests for my changes.
- [x] I have tested my changes.
- [ ] I have updated the project documentation to reflect my changes.
- [ ] I have read the [CONTRIBUTING](https://github.com/Microsoft/fast/blob/master/CONTRIBUTING.md) documentation and followed the [standards](https://www.fast.design/docs/community/code-of-conduct/#our-standards) for this project.

### Component-specific

<!--- Review the list and put an x in the boxes that apply. -->
<!--- Remove this section if not applicable. -->

- [ ] I have added a new component
- [ ] I have modified an existing component
- [ ] I have updated the [definition file](https://github.com/Microsoft/fast/blob/master/packages/web-components/fast-components/CONTRIBUTING.md#definition)
- [ ] I have updated the [configuration file](https://github.com/Microsoft/fast/blob/master/packages/web-components/fast-components/CONTRIBUTING.md#configuration)

## ⏭ Next Steps

<!---
If there is relevant follow-up work to this PR, please list any existing issues or provide brief descriptions of what you would like to do next.
-->
@chrisdholt
update api report
e23a0ab
@chrisdholt
remove unused vars from fast-web-utilities
54c5c13
@chrisdholt
remove yarn cache to unblock the build due to caching issue
868333d
@chrisdholt
update ts-jest and replace awesome-typescript-loader with ts-loader t…
08439bf 
…o fix fast-animation issue
@chrisdholt chrisdholt force-pushed the users/chhol/update-typescript-to-4.6.2 branch from b6bcb61 to 08439bf Compare April 26, 2022 20:11
@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://blue-coast-0df7a6610-5788.centralus.azurestaticapps.net

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-sand-03fe10e10-5788.centralus.azurestaticapps.net

1 similar comment
@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-sand-03fe10e10-5788.centralus.azurestaticapps.net

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://blue-coast-0df7a6610-5788.centralus.azurestaticapps.net

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-5788.centralus.azurestaticapps.net

1 similar comment
@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-5788.centralus.azurestaticapps.net

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-5788.centralus.azurestaticapps.net

@chrisdholt chrisdholt merged commit 35bdab4 into master Apr 26, 2022
@chrisdholt chrisdholt deleted the users/chhol/update-typescript-to-4.6.2 branch April 26, 2022 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williamw2 williamw2 williamw2 approved these changes

@nicholasrice nicholasrice nicholasrice approved these changes

@radium-v radium-v radium-v approved these changes

@EisenbergEffect EisenbergEffect EisenbergEffect approved these changes

@janechu janechu Awaiting requested review from janechu

@bheston bheston Awaiting requested review from bheston

@awentzel awentzel Awaiting requested review from awentzel

@Falkicon Falkicon Awaiting requested review from Falkicon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fix: Cannot use TypeScript > 4.3.5 when importing FAST components
6 participants
@chrisdholt @nicholasrice @EisenbergEffect @radium-v @awentzel @williamw2