vetur icon indicating copy to clipboard operation
vetur copied to clipboard

Apply template interpolation validation only to type-checked components

Open jackkoppa opened this issue 4 years ago • 12 comments

Addresses #1427

Even when vetur.experimental.templateInterpolationService: true, the nature of Vue 2 this reliance means that completely valid JS components will still show errors in the template section, because plain TS inference will not work without Vue.extend({}) and certain type annotations For this reason, Vetur doesn't attempt to provide type checking in the

  1. component uses lang="ts"
  2. components includes // @ts-check comment (& uses JSDoc for type annotations)
  3. component imports a .ts external script using src="" (thus the type checking happens in that file)

Template interpolation should now work the same way (should only be applied in the above 3 scenarios), to allow projects with a mixture of type-checked & non-type-checked components, to still turn templateInterpolationService on, and get benefits in the type-checked components, without getting lots of "incorrect" errors in non-type-checked components

jackkoppa avatar Apr 05 '20 23:04 jackkoppa

Noted that the build failed on Windows; will run locally on a Windows machine to fix the file path resolution

jackkoppa avatar Apr 05 '20 23:04 jackkoppa

Checks now pass, and would love to discuss with @ktsn or @octref, whenever someone is available - I'm sure I missed things about how these checks could be done more easily. Thanks!

(I'm hopeful this could get us to the point where we could consider moving templateInterpolationService out from the experimental flag, and possibly turning on by default, to start exposing more devs to the feature. Given how well it's worked for us in our type-checked files - we have yet to run into any bugs/incorrect errors in those. Though I know that's a later discussion)

cc @F0rsaken

jackkoppa avatar Apr 06 '20 05:04 jackkoppa

Checks now pass, and would love to discuss with @ktsn or @octref, whenever someone is available - I'm sure I missed things about how these checks could be done more easily. Thanks!

(I'm hopeful this could get us to the point where we could consider moving templateInterpolationService out from the experimental flag, and possibly turning on by default, to start exposing more devs to the feature. Given how well it's worked for us in our type-checked files - we have yet to run into any bugs/incorrect errors in those. Though I know that's a later discussion)

cc @F0rsaken

I seriously can't wait for it to go live and I can start using interpolationService in mixed projects! Thanks fo contribution @jackkoppa!

F0rsaken avatar Apr 06 '20 12:04 F0rsaken

Thank you for making this working @jackkoppa. Please let me get some time to take a look at this as I'm currently kind of overwhelmed by my day job 😅

I'm hopeful this could get us to the point where we could consider moving templateInterpolationService out from the experimental flag, and possibly turning on by default, to start exposing more devs to the feature.

This PR definitely makes it much closer to the point removing experimental flag but I would say we also need to solve #1212, which would be pretty annoying for users, before do that.

ktsn avatar Apr 13 '20 03:04 ktsn

Thanks @ktsn! Totally understand that you've got a lot going on.

but I would say we also need to solve #1212, which would be pretty annoying for users, before do that

In looking at #1208, I'm not confident I'd be able to understand/help much with #1212, but it's good to know that's another high priority for template interpolation. I'll try to take a look if I get a chance at some point

jackkoppa avatar Apr 13 '20 13:04 jackkoppa

Hi @octref - great to see another Vetur release happen!

I can get back to this, to resolve conflicts and, optionally, to open the TS repo issue I mentioned to expose an API for isUncheckedFile. Anything else you'd like to see before this is merge-able?

jackkoppa avatar Aug 04 '20 12:08 jackkoppa

@jackkoppa I'm working on projects that are using SFC written in JS where I don't use Vue.extend but I have global shim to set the *.vue type correctly:

declare module '*.vue' {
    import Vue from 'vue';
    export default Vue;
}

And I would like this to continue working without having to add @ts-check in every Vue file.

rchl avatar Aug 04 '20 12:08 rchl

Anything else you'd like to see before this is merge-able?

Sorry I haven't had a close look yet 😅 Will continue tomorrow. Planning to finish all other PRs before doing multiroot and sub-folder support. Just resolving the conflict is fine for now 👍

octref avatar Aug 04 '20 12:08 octref

Thanks @octref, will work on resolving the conflicts

@rchl, interesting - I'm really surprised that that works effectively with no JSDoc annotations & no TS files, given things like computed props & their self-referential return types (which have required some kind of annotation, in all my Vue 2 experiences). Any sample repo you might be able to link to? The good news is that the TS method we'd hope to use if made available, isUncheckedFile, also checks whether compilerOptions.checkJs is enabled, in which case not every file would need @ts-check; just defined once in tsconfig

Likely won't work as this PR stands, so would love to see a sample repo?

jackkoppa avatar Aug 05 '20 01:08 jackkoppa

@rchl, interesting - I'm really surprised that that works effectively with no JSDoc annotations

@jackkoppa

I did not say that it works without JSDoc annotations. :) I have checkJs enabled in tsconfig.json, I have the aforementioned shim and I'm adding JSDoc annotations everywhere where it's needed.

I don't have a public repo to show but here is one of the (redacted) components:

<template>
    <div>
        <p>{{ headerText }}</p>
        <p v-if="enabledFields.includes('name')">Something</p>
    </div>
</template>

<script>
import fieldName from '~/components/field-name.vue';

export default {
    components: {
        fieldName,
    },
    props: {
        /** @type {import('vue').PropOptions<string[]>} */
        enabledFields: {
            type: Array,
            required: true,
            validator: values => values.every(v => ['a', 'b', 'c'].includes(v)),
        },
        headerText: {
            type: String,
            default: '',
        },
    },
    computed: {
        /** @return {string} */
        something() {
            return 'sss'
        },
    },
    methods: {
        /**
         * @param {MouseEvent} event
         * @param {MenuItem} item
         */
        handleClick(event, item) {
            event.preventDefault();
        },
    },
};
</script>

The tsconfig.json looks more or less like this:

{
    "compilerOptions": {
        "baseUrl": ".",
        "target": "esnext",
        "module": "esnext",
        "esModuleInterop": true,
        "moduleResolution": "node",
        "noEmit": true,
        "allowJs": true,
        "checkJs": true,
        "allowSyntheticDefaultImports": true,
        "strict": true,
        "resolveJsonModule": true,
        "lib": [
            "dom",
            "esnext",
        ],
    }
}

rchl avatar Aug 05 '20 07:08 rchl

Can you tick "allow edits from maintainers", or do a rebase off origin/master? Thanks 👍

octref avatar Aug 26 '20 07:08 octref

@jackkoppa Any updates on when will it be finally merged?

HaimCandiTech avatar Aug 22 '22 12:08 HaimCandiTech

Hi all - it's been a couple years since I've worked closely with Vue + TypeScript in my day-to-day work, and so it'll be a little tricky for me to pick this back up / resolve conflicts / make the improvements mentioned in the comments. When I had left off with this a couple years ago, the PR did work, but I also imagine with continued Vue + Vetur changes, there might be tweaks needed.

I don't want to contribute to long-stale PRs, so I'll be closing this for the time being. If I do end up in a Vue + TS codebase in the near future & still experience a similar issue, I'd come back to this, and anyone else is welcome to pick up the branch as well. Thanks all!

jackkoppa avatar Dec 20 '22 20:12 jackkoppa