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

Projects
@ferdaber
Copy link
Contributor

ferdaber commented Oct 19, 2017

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:

@ktsn

ktsn approved these changes Oct 19, 2017

@HerringtonDarkholme

This comment has been minimized.

Copy link
Member

HerringtonDarkholme commented Oct 19, 2017

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

This comment has been minimized.

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

@HerringtonDarkholme
Copy link
Member

HerringtonDarkholme left a comment

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

@ktsn

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

HerringtonDarkholme commented Oct 20, 2017

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

@ferdaber

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

yyx990803 commented Oct 20, 2017

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

@ferdaber

This comment has been minimized.

Copy link
Contributor Author

ferdaber commented Oct 20, 2017

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>[];

This comment has been minimized.

@thatguystone

thatguystone Mar 27, 2018

Should the type here be PropType<T>?

ferdaber added some commits Mar 27, 2018

@alexsasharegan

This comment has been minimized.

Copy link

alexsasharegan commented Apr 4, 2018

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

This comment has been minimized.

Copy link

cesalberca commented Apr 19, 2018

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

@KindWizzard

This comment has been minimized.

Copy link

KindWizzard commented Apr 23, 2018

Any updates on this?

@ascii-soup

This comment has been minimized.

Copy link

ascii-soup commented May 21, 2018

Any movement on this? Keen to see it land!

@Grandizer

This comment has been minimized.

Copy link

Grandizer commented May 30, 2018

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

@yordis

This comment has been minimized.

Copy link

yordis commented Jun 9, 2018

@sodatea Do you know when this will be released?

@pushkarskiy

This comment has been minimized.

Copy link

pushkarskiy commented Jul 4, 2018

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

@yyx990803

This comment has been minimized.

Copy link
Member

yyx990803 commented Jul 30, 2018

Once we finish CLI release this will be the first to be merged in core, please bear with us.

@ffxsam

This comment has been minimized.

Copy link

ffxsam commented Aug 10, 2018

@yyx990803 Huge congrats on the Vue CLI release! It's amazing.

Now you know what I'm going to ask next.. 😉 It would be awesome if this could be merged in. All over our huge codebase we just adapted to TypeScript, we've had to comment out type: Array on all our props.

@RehanSaeed

This comment has been minimized.

Copy link

RehanSaeed commented Aug 20, 2018

Just want to echo the question by @alexsasharegan. Will it solve that problem? Are there any workarounds?

@r0skar

This comment has been minimized.

Copy link

r0skar commented Aug 23, 2018

@RehanSaeed Yes there is. Check out this great article by @mmitchellgarcia.

@ffxsam

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

ffxsam commented Sep 1, 2018

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

@cpmech

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

cesalberca commented Oct 2, 2018

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

This comment has been minimized.

Copy link

sjmcdowall commented Oct 2, 2018

@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

This comment has been minimized.

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

This comment has been minimized.

Copy link

cesalberca commented Oct 2, 2018

@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

This comment has been minimized.

Copy link

japboy commented Oct 17, 2018

will this be merged so soon?

@yyx990803 yyx990803 added this to In progress in 2.6 Dec 5, 2018

@yyx990803 yyx990803 changed the base branch from dev to 2.6 Dec 5, 2018

@yyx990803 yyx990803 merged commit 051ea8c into vuejs:2.6 Dec 5, 2018

1 check was pending

ci/circleci: install CircleCI is running your tests
Details

2.6 automation moved this from In progress to Done Dec 5, 2018

f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019

@abelgoodwin1988

This comment has been minimized.

Copy link

abelgoodwin1988 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.