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

Apply template interpolation validation only to TypeScript components #1439

Draft
wants to merge 3 commits into
base: master
from

Conversation

@jackkoppa
Copy link

commented Sep 22, 2019

Hi @ktsn - thanks for the direction on this in #1427

The PR checks if the current document includes typescript as a language; if it does not, the template interpolation service will no longer attempt to do validation on that document. As you suggested here, though, the other, non-diagnostic features associated with the interpolation service will continue to run, so long as the user has enabled the feature in their settings.

What I still need assistance with:

  • I'm not certain how we should be checking for // @ts-check. Do you happen to know if the typescript module provides information about "yes, this file is being type-checked, whether because of its extension or the ts-check directive"? Otherwise, presumably we need to parse the beginning of the script, to find that actual text?
  • I wanted to confirm that the method of checking the languages in the current document is OK; after a fair amount of digging, I couldn't find an alternative approach in the codebase

Please absolute suggest any changes or alternative approaches; just wanted to get an initial approach out there. There are currently 2 failing interpolation tests; the JSDoc case can certainly be handled if we figure out how to capture the ts-check directive

jackkoppa added 3 commits Sep 22, 2019
Add method in Vue info service to check whether a given language exists in the current document
Use that method to apply template interpolation checks only when TypeScript is found
Resolves #1427

TODO: Add specs if possible
Allow all other interpolation services to continue to run,
even when the component does not have TypeScript enabled
Because no TypeScript enabled in the component, it should still have all hover & references functionality. But no validation errors should display
@jackkoppa

This comment has been minimized.

Copy link
Author

commented Sep 22, 2019

(should not be merged until we at least address the failing specs, and hopefully the ts-check case)

The known failing specs:

    1) Shows template diagnostics for jsdocs-type-check.vue

    2) Shows template diagnostics for external-script.vue

(also, as an aside - major hats off for the extremely thorough test suite, auto-linting, tests-on-push, thorough typing everywhere, and CI setup. A joy of a codebase to contribute to)

@ktsn

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

Thank you for the PR! I'll look into the details when I have more extra time.

checking for // @ts-check

Looks like we can get this info from SourceFile#checkJsDirective.
https://ts-ast-viewer.com/#code/FAehAIAEBcGcFoDGALApog1sRB7AdrNOAIbgC84A5JcEA

We still need to think about how we get source file object in doValidation.

I wanted to confirm that the method of checking the languages in the current document is OK

Looks good to me.

private interpolationEnabled(): boolean {
return _.get(this.config, ['vetur', 'experimental', 'templateInterpolationService'], true);
}
private documentTypeChecked(document: TextDocument): boolean {

This comment has been minimized.

Copy link
@jackkoppa

jackkoppa Sep 30, 2019

Author

Thanks for the response thus far, @ktsn. Very much appreciated.

I'll come back to this in the near future, but for now, I'm having trouble accessing sourceFile.checkJsDirective; it shows as non-existent when I do the following:

  private documentTypeChecked(document: TextDocument): boolean {
    // Very rough changes, just to see if we can get the right AST 
    // without changing providers or services for this class right away

    const { updateCurrentVueTextDocument } = this.serviceHost;
    const { service } = updateCurrentVueTextDocument(document);
    const program = service.getProgram();
    if (!languageServiceIncludesFile(service, document.uri) || !program) {
      return false;
    }
    const fileFsPath = getFileFsPath(document.uri);
    const sourceFile = program.getSourceFile(fileFsPath);
    
    return !!this.vueInfoService && (
      this.vueInfoService.documentIncludesLanguage(document, 'typescript') ||
      // ISSUE: Below, sourceFile, type `SourceFile`, does not contain `checkJsDirective` 
      (sourceFile && (sourceFile as any).checkJsDirective.enabled));
  }

This comment has been minimized.

Copy link
@jackkoppa

jackkoppa Sep 30, 2019

Author

Feel free to copy/paste these changes into the branch to see what I mean.
TypeScript version seems fine; it's odd to me that the AST viewer includes checkJsDirective, but it's not a property available on the SourceFile interface.

I'll come back to this when I have time; just leaving the note in case anyone's interested in where I'm currently stuck

This comment has been minimized.

Copy link
@ktsn

ktsn Oct 1, 2019

Member

This looks like not in declaration but available in runtime. Using it with type cast would be OK I think.

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