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.

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

refactor: update to typescript 3.9 #9594

Merged
merged 23 commits into from
Jul 22, 2020
Merged

refactor: update to typescript 3.9 #9594

merged 23 commits into from
Jul 22, 2020

Conversation

KaelWD
Copy link
Member

@KaelWD KaelWD commented Nov 6, 2019

Had to skip a bunch of tests that started failing for no reason, if anyone else wants to have a go at fixing them be my guest.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any features but makes things better)

@KaelWD KaelWD added the S: on hold The issue is on hold until further notice label Nov 6, 2019
@TravisBuddy
Copy link

TravisBuddy commented Nov 6, 2019

Hey @KaelWD,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: ba3de7f0-cbe5-11ea-acda-6fc69b852bf0

@KaelWD KaelWD added the help wanted We are looking for community help label Nov 7, 2019
@KaelWD KaelWD added the S: has merge conflicts The pending Pull Request has merge conflicts label Jan 28, 2020
@KaelWD KaelWD force-pushed the master branch 3 times, most recently from 9e00958 to de967ac Compare February 5, 2020 07:33
@johnleider
Copy link
Member

Is this still being worked on?

@johnleider
Copy link
Member

Pulse check.

@KaelWD
Copy link
Member Author

KaelWD commented Apr 9, 2020

Still busted, 3.8.3 seems to work though

@KaelWD KaelWD changed the title refactor: update to typescript 3.7 refactor: update to typescript 3.8 Apr 9, 2020
@KaelWD KaelWD removed S: has merge conflicts The pending Pull Request has merge conflicts S: on hold The issue is on hold until further notice labels Apr 9, 2020
@jacekkarczmarczyk jacekkarczmarczyk added the S: has merge conflicts The pending Pull Request has merge conflicts label May 6, 2020
@johnleider
Copy link
Member

Is this still being worked on? v3 work is about to start heavily.

@KaelWD KaelWD modified the milestones: v3.0.0, v2.3.x Jun 12, 2020
@KaelWD KaelWD removed S: on hold The issue is on hold until further notice help wanted We are looking for community help labels Jun 12, 2020
@KaelWD KaelWD requested a review from a team June 12, 2020 06:00
@KaelWD KaelWD added pending team review The issue is pending a full team review task labels Jun 12, 2020
@KaelWD KaelWD requested a review from a team June 25, 2020 10:43
@@ -117,7 +117,8 @@ describe('VTreeView.ts', () => { // eslint-disable-line max-statements
expect(wrapper.html()).toMatchSnapshot()
})

it('should load children when expanding', async () => {
// TODO: fails with TS 3.9
Copy link
Member

Choose a reason for hiding this comment

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

This is quite a lot of coverage loss

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep

@johnleider
Copy link
Member

Requesting Final comments from @vuetifyjs/contributors @vuetifyjs/core-team

@@ -323,7 +323,7 @@ export default mixins(
consoleWarn(`Value must be ${this.isMultiple ? 'an' : 'a'} ${expected}, got ${valueType}`, this)
}
},
isDateAllowed (value: string) {
isDateAllowed (value: string): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

This type annotation can be removed as others were for consistency, can't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for some ones lower (I think this.$createElement has types)

Copy link
Member Author

@KaelWD KaelWD Jul 13, 2020

Choose a reason for hiding this comment

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

Nope, it breaks this without them. The return type inference seems to be getting worse with each version, you used to be able to have computeds without an explicit return type but now it's needed for methods sometimes too.

Copy link
Contributor

Choose a reason for hiding this comment

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

😞 okay, that's really bad. I only remember broken this w/o computed types. Maybe we should open an issue to Vue?

@KaelWD KaelWD removed the pending team review The issue is pending a full team review label Jul 22, 2020
@KaelWD KaelWD self-assigned this Jul 22, 2020
@KaelWD KaelWD merged commit 0335c6d into master Jul 22, 2020
@KaelWD KaelWD deleted the refactor/ts37 branch July 22, 2020 06:41
KaelWD added a commit that referenced this pull request Jul 23, 2020
KaelWD added a commit that referenced this pull request Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants