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

feat: add Prop to main type declaration file [#6850] #6856

Merged
merged 5 commits into from
Dec 5, 2018

Conversation

ferdaber
Copy link
Contributor

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

See #6850.

Other information:

@HerringtonDarkholme
Copy link
Member

This should also fix #6841.

I wonder how it would impact case like union: [User, Number].

Of course, exporting more types means more compatibility for us to maintain. If it can also fix union case I think we should ship this feature.

@ferdaber
Copy link
Contributor Author

ferdaber commented Oct 19, 2017

@HerringtonDarkholme it seems to work fine with union types where you want the constructor instead of the primitive:
union
unionalso

Copy link
Member

@HerringtonDarkholme HerringtonDarkholme left a comment

Choose a reason for hiding this comment

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

Thanks! This pull request does fix some problems!
union props can be fully typed now!

@ktsn
Copy link
Member

ktsn commented Oct 20, 2017

To deal with union type, I guess it would be more convenient to declare another type (e.g. PropType in the following example) to unify the casting syntax.

type PropType<T> = Prop<T> | Prop<T>[]

interface Foo {
  a: string
}

Vue.extend({
  props: {
    // Both looks the same syntax
    // (we don't need to concern the `type` is an array or not)
    foo: Object as PropType<Foo>
    union: [Object, String] as PropType<Foo | string>
  }
})

@HerringtonDarkholme
Copy link
Member

Prop<string | Foo>[] is enough for union case. This is both concise and saves us one exported type.

@ferdaber
Copy link
Contributor Author

ferdaber commented Oct 20, 2017

@HerringtonDarkholme, I think that @ktsn has a good point. That is technically the purpose of PropValidator<T> which serves as the all-encompassing union of PropOptions<T> | Prop<T> | Prop<T>[]. Would it be better to export PropValidator<T> instead so that users don't have to worry about conforming to the Vue prop validator syntax, and just do:

props: {
  myProp: {/* constructor, array, or prop options */} as PropValidator<IMyProp>
}

@ktsn
Copy link
Member

ktsn commented Oct 20, 2017

@HerringtonDarkholme
I didn't mean we should export both Prop and PropType - I think we should minimize exposing types as possible too.

But the Prop might be a little low level to be used in user land because we need to consider whether the props type is an array or not, even though we just want to focus its type parameter in that case.
So I would suggest to add a thin wrapper type to intended to be used in the user land.

@ferdaber
I'd say PropValidator shouldn't be exposed since we just want to cast type here. It seems a little overkill for me.

@ferdaber
Copy link
Contributor Author

ferdaber commented Oct 20, 2017

The only difference between PropType<T> (which currently does not exist in options.d.ts), and PropValidator<T> is the additional union of the customized object syntax of the prop validator API. If someone were to use the customized object syntax then they would still need to do something like:

props: {
  union: [Object, String] as PropType<Foo | string>,
  complexUnion: {
    type: [Object, String] as PropType<Foo | string>
    // ... other stuff
  }
}

versus

props: {
  union: [Object, String] as PropValidator<Foo | string>,
  complexUnion: {
    type: [Object, String]
    // ... other stuff
  } as PropValidator<Foo | string>
}

I guess to me it makes more sense to have the typecast be in the same level of each prop definition, but it doesn't really make much of a difference so I'm fine with whichever you all decide. The other benefit I see too would be that PropValidator is the more abstract definition of props in a Vue constructor, so if in the future the lower-level API changes PropValidator is the one that's least likely to change

@yyx990803
Copy link
Member

I think PropType is more pragmatic than Prop and more intuitive than PropValidator.

@ferdaber
Copy link
Contributor Author

Great! I will modify the PR to create a new type called PropType and export that in place of Prop

export type PropValidator<T> = PropOptions<T> | Prop<T> | Prop<T>[];
export type PropType<T> = Prop<T> | Prop<T>[];

export type PropValidator<T> = PropOptions<T> | PropType<T>;

export interface PropOptions<T=any> {
type?: Prop<T> | Prop<T>[];

Choose a reason for hiding this comment

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

Should the type here be PropType<T>?

@alexsasharegan
Copy link

Does this PR solve the prop compile error: TS2339: Property 'x' does not exist on type 'CombinedVueInstance<Vue, ...>?

Compiler Error

ERROR in src/js/components/Manage/DataViz/HdSalesGraph.vue.ts
[tsl] ERROR in src/js/components/Manage/DataViz/HdSalesGraph.vue.ts(71,26)
      TS2339: Property 'currentMoment' does not exist on type 'CombinedVueInstance<Vue, { styles: { width: string; height: string; position: string; }; }, { for...'.

ERROR in src/js/components/Manage/DataViz/HdSalesGraph.vue.ts
[tsl] ERROR in src/js/components/Manage/DataViz/HdSalesGraph.vue.ts(75,26)
      TS2339: Property 'previousMoment' does not exist on type 'CombinedVueInstance<Vue, { styles: { width: string; height: string; position: string; }; }, { for...'.

Abbreviated Source

<script lang="ts">
import Vue from "vue";
import { Prop } from "vue/types/options";
import moment from "moment";

export default Vue.extend({
  name: "HdSalesGraph",

  props: {
    currentMoment: {
      type: Object as Prop<moment.Moment>,
      required: true,
    },

    previousMoment: {
      type: Object as Prop<moment.Moment>,
      required: true,
    },
  },

  data() {
    return {
      styles: {
        width: "100%",
        height: "500px",
        position: "relative",
      },
    };
  },

  computed: {
    curMoment(): moment.Moment {
      return moment(this.currentMoment);
      //                 ^^^^^^^^^^^^^ 71,26 
    },

    prevMoment(): moment.Moment {
      return moment(this.previousMoment);
      //                 ^^^^^^^^^^^^^^ 75,26
    },
  },
});
</script>

@cesalberca
Copy link

Will this change solve this issue: #7640? Because in the TypeScript repo they've said that it works as intended: microsoft/TypeScript#22471

@KindWizzard
Copy link

Any updates on this?

@ascii-soup
Copy link

Any movement on this? Keen to see it land!

@Grandizer
Copy link

Do we know when this will be released? I am sure "when it's ready" but just looking for a guestimate?

@yordis
Copy link

yordis commented Jun 9, 2018

@sodatea Do you know when this will be released?

@pushkarskiy
Copy link

Do we know when this will be released? This very important for us.

@ffxsam
Copy link

ffxsam commented Aug 28, 2018

@yyx990803 It's been a month since your last comment. Any hope of getting this merged soon? I'm glad there's a workaround (by simply excluding type: Array), but it's still a nuisance to not have proper type checking for props.

@ky-is
Copy link

ky-is commented Aug 30, 2018

@ffxsam Looking forward to this as well, but is the function interface syntax not a viable workaround? Types are working for me with:

type: Array as () => string[],

@ffxsam
Copy link

ffxsam commented Sep 1, 2018

@ky-is Nice trick, I'll give it a try. Thanks!

@cpmech
Copy link

cpmech commented Sep 3, 2018

Hi, I'm facing this problem as well. The code is pretty simple:

<script lang="ts">
import Vue from 'vue';
export default Vue.extend({
  props: {
    figs: Array,
  },
  data() {
    return {
      isModal: false,
    };
  },
  methods: {
    closeModal() {
      this.isModal = false;
    },
  },
});
</script>

I get the following error (on line this.isModal = false):

Property 'isModal' does not exist on type 'Vue'.

Hope we'll find a solution soon.

@ffxsam
Copy link

ffxsam commented Sep 3, 2018

The solution is this PR. I'm not sure what the holdup is. Is it pending review?

@kjleitz
Copy link

kjleitz commented Oct 1, 2018

@yyx990803 @ferdaber can this be merged in? It's a big roadblock for our TypeScript codebase, too!

@cesalberca
Copy link

I think what's happening is that they won't focus on this kind of issues as they are rewritting Vue in TypeScript (as announced here: https://medium.com/the-vue-point/plans-for-the-next-iteration-of-vue-js-777ffea6fabf).

@sjmcdowall
Copy link

@cesalberca -- that is all well and good, but realistically it may be a year before adoption of V3.0 etc. meanwhile people everyday are going through this pain -- which is FIXED -- and an easy patch. It doesn't make sense.

@wanton7
Copy link

wanton7 commented Oct 2, 2018

Maybe just just make a local copy and patch it yourself and not wait forever? That what we did.

@cesalberca
Copy link

@cesalberca -- that is all well and good, but realistically it may be a year before adoption of V3.0 etc. meanwhile people everyday are going through this pain -- which is FIXED -- and an easy patch. It doesn't make sense.

I know @sjmcdowall , I'm just saying that this is what may be occurring. I wouldn't agree too if this is whats truly happening.

@japboy
Copy link

japboy commented Oct 17, 2018

will this be merged so soon?

@yyx990803 yyx990803 changed the base branch from dev to 2.6 December 5, 2018 22:47
@yyx990803 yyx990803 merged this pull request into vuejs:2.6 Dec 5, 2018
f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019
@hampgoodwin
Copy link

hampgoodwin commented Feb 20, 2019

Fantastic!
Thanks for all the hard work and merging this.
Is there some additional documentation on this somewhere so I can double make sure I'm implementing this correctly? I have:

<script lang="ts">
import Vue from 'vue';
import { Prop } from 'vue/types/options';
import { Education } from '@/assets/resume/resume.model';

export default Vue.extend({
    props: {
        education: Array as Prop<Education[]>,
    },
});

Is this the appropriate way to do it?

@LissetteIbnz
Copy link

@abelgoodwin1988, I think you should use it like that

<script lang="ts">
import Vue, { PropType } from 'vue';
import { Education } from '@/assets/resume/resume.model';

export default Vue.extend({
    props: {
        education: Array as PropType<Education[]>,
    },
});

The PropType interface definition is export type PropType<T> = Prop<T> | Prop<T>[];

Fantastic!
Thanks for all the hard work and merging this.
Is there some additional documentation on this somewhere so I can double make sure I'm implementing this correctly? I have:

<script lang="ts">
import Vue from 'vue';
import { Prop } from 'vue/types/options';
import { Education } from '@/assets/resume/resume.model';

export default Vue.extend({
    props: {
        education: Array as Prop<Education[]>,
    },
});

Is this the appropriate way to do it?

@alexsasharegan
Copy link

alexsasharegan commented May 19, 2019

I've been having good luck with PropOptions as an alternative approach.

<script lang="ts">
import Vue, { PropOptions } from 'vue';
import { Education } from '@/assets/resume/resume.model';

export default Vue.extend({
    props: {
        education: Array,
    } as PropOptions<Education[]>,
});

The name might be singular, I can't check now from my phone. I like this approach because it also hints the validator function parameters.

@liulei237136
Copy link

ok

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.