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: convert components to typescript #3837

Merged
merged 26 commits into from May 10, 2018
Merged

refactor: convert components to typescript #3837

merged 26 commits into from May 10, 2018

Conversation

KaelWD
Copy link
Member

@KaelWD KaelWD commented Apr 12, 2018

Description

This should be ready to go, I've got tests and builds working with no issues as far as I can tell, and it's actually smaller than the current dev build by ~8KB minified.

So far I've converted VIcon, VBtn, VbtnToggle, and a few mixins and helpers. Everything else can be done incrementally over time.

Possible component patterns:
// Regular options object
export default Vue.extend({
  ...
})

// Or with mixins
export default mixins(Foo, Bar).extend({
  ...
})
// Class with decorators, makes props a bit messy
@Component({
  props: {
    foo: String,
    bar: Boolean
  }
})
export default class FooBar extends Vue {
  foo: string
  bar: boolean
}

// Or
@Component
export default class FooBar extends Vue {
  @Prop() foo: string
  @Prop() bar: boolean
}
// Both?
const options = Vue.extend({
  props: {
    foo: String,
    bar: Boolean
  }
})

@Component
export default class FooBar extends options {
  ... // No need to declare props twice
}

 
Things to note:

  • Components are now vue constructors, not plain objects. This means you have to use SomeComponent.options to access properties on the original object.
  • To convert a component to TS, you must also convert everything that it imports. Probably best to start with mixins and smaller components so you don't have to do too much at once.
  • Functional components currently don't inherit prop types from their mixins, which is why I've had to declare them manually in VIcon.

Fixes #3943

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 feature but make things better)

TODO:

  • The rest of the components
  • Check if sourcemaps are still good, considering it goes through two transpilers now
  • Provide/inject with more than just register/unregister
  • $refs declarations
  • Tests
  • Prod build
  • es5 build
  • custom linting rules

@KaelWD KaelWD added T: enhancement Functionality that enhances existing features task labels Apr 12, 2018
@KaelWD KaelWD self-assigned this Apr 12, 2018
@KaelWD KaelWD added pending team review The issue is pending a full team review and removed in progress labels May 3, 2018
@codecov
Copy link

codecov bot commented May 9, 2018

Codecov Report

Merging #3837 into dev will decrease coverage by 0.03%.
The diff coverage is 93.2%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #3837      +/-   ##
==========================================
- Coverage   87.22%   87.19%   -0.04%     
==========================================
  Files         154      158       +4     
  Lines        3782     3990     +208     
  Branches     1200     1272      +72     
==========================================
+ Hits         3299     3479     +180     
- Misses        371      396      +25     
- Partials      112      115       +3
Impacted Files Coverage Δ
src/components/Vuetify/index.js 100% <ø> (ø) ⬆️
src/components/VDivider/VDivider.js 100% <ø> (ø) ⬆️
src/components/VAvatar/VAvatar.js 100% <ø> (ø) ⬆️
src/components/VBtn/index.ts 100% <100%> (ø)
src/mixins/toggleable.ts 100% <100%> (ø)
src/components/VIcon/VIcon.ts 91.93% <100%> (ø)
src/util/mixins.ts 100% <100%> (ø)
test/index.js 83.92% <100%> (+0.9%) ⬆️
src/mixins/colorable.ts 96.77% <100%> (ø)
src/components/VBtnToggle/index.ts 100% <100%> (ø)
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f679af6...10c86d1. Read the comment docs.

@KaelWD KaelWD merged commit 10c86d1 into dev May 10, 2018
@KaelWD KaelWD deleted the refactor/typescript branch May 10, 2018 16:16
@KaelWD KaelWD removed the pending team review The issue is pending a full team review label May 10, 2018
@jsaak
Copy link

jsaak commented May 31, 2018

It may be just me, but it seems this work is pretty useless.
If someone wants typescript then there is angular.

@jvdwijngaard
Copy link

I don't agree.

Vue is compatible with TypeScript as well, but Vuetify was not entirely ready for that.
The use of TypeScript has nothing to do with Angular. Thát's actually why I love Vue, they give you a choice in what you want to use, instead of forcing users to use TypeScript.

This helps using the Vuetify framework in TypeScript enabled Vue projects and still have the possibility to import only components you need/use (a-la-carte), instead of having to import the entire set of components and corresponding styling, keeping the output files as small as possible.

Ofcourse this is just a start and like Kael said, other components need to be converted, but I'm really happy this is happening.

@jsaak
Copy link

jsaak commented May 31, 2018

As long as using typescript remains optional I do agree.
But reading the notes:

Components are now vue constructors, not plain objects. This means you have to use
SomeComponent.options to access properties on the original object.

Functional components currently don't inherit prop types from their mixins, which is why I've had to >declare them manually in VIcon.

I had the feeling that it would be NOT optional.

@KaelWD
Copy link
Member Author

KaelWD commented May 31, 2018

This makes 0 difference to you unless you are working on vuetify itself.

@lock
Copy link

lock bot commented Apr 15, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. Please direct any non-bug questions to our Discord

@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T: enhancement Functionality that enhances existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants