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.

Sign up for GitHub

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

Jump to bottom

Apply template interpolation validation only to TypeScript components #1439

Closed
wants to merge 3 commits into from
Closed

Apply template interpolation validation only to TypeScript components #1439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
57a409f
Enable template interpolation only for TS components
jackkoppa Sep 22, 2019
d7d57ea
Make only validation contingent on presence of TypeScript
jackkoppa Sep 22, 2019
6ea759f
Add interpolation test for component w/ type error, but no TS checking
jackkoppa Sep 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/src/modes/template/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export class VueHTMLMode implements LanguageMode {
) {
const vueDocuments = getLanguageModelCache<HTMLDocument>(10, 60, document => parseHTMLDocument(document));
this.htmlMode = new HTMLMode(documentRegions, workspacePath, vueDocuments, vueInfoService);
this.vueInterpolationMode = new VueInterpolationMode(tsModule, serviceHost, vueDocuments);
this.vueInterpolationMode = new VueInterpolationMode(tsModule, serviceHost, vueDocuments, vueInfoService);
}
getId() {
return 'vue-html';
Expand Down
19 changes: 14 additions & 5 deletions server/src/modes/template/interpolationMode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,16 @@ import { toCompletionItemKind } from '../../services/typescriptService/util';
import { LanguageModelCache } from '../../embeddedSupport/languageModelCache';
import { HTMLDocument } from './parser/htmlParser';
import { isInsideInterpolation } from './services/isInsideInterpolation';
import { VueInfoService } from '../../services/vueInfoService';

export class VueInterpolationMode implements LanguageMode {
private config: any = {};

constructor(
private tsModule: T_TypeScript,
private serviceHost: IServiceHost,
private vueDocuments: LanguageModelCache<HTMLDocument>
private vueDocuments: LanguageModelCache<HTMLDocument>,
private vueInfoService?: VueInfoService
) {}

getId() {
Expand All @@ -48,7 +50,7 @@ export class VueInterpolationMode implements LanguageMode {
}

doValidation(document: TextDocument): Diagnostic[] {
if (!_.get(this.config, ['vetur', 'experimental', 'templateInterpolationService'], true)) {
if (!this.interpolationEnabled || !this.documentTypeChecked(document)) {
return [];
}

Expand Down Expand Up @@ -216,7 +218,7 @@ export class VueInterpolationMode implements LanguageMode {
contents: MarkedString[];
range?: Range;
} {
if (!_.get(this.config, ['vetur', 'experimental', 'templateInterpolationService'], true)) {
if (!this.interpolationEnabled()) {
return { contents: [] };
}

Expand Down Expand Up @@ -255,7 +257,7 @@ export class VueInterpolationMode implements LanguageMode {
}

findDefinition(document: TextDocument, position: Position): Location[] {
if (!_.get(this.config, ['vetur', 'experimental', 'templateInterpolationService'], true)) {
if (!this.interpolationEnabled()) {
return [];
}

Expand Down Expand Up @@ -303,7 +305,7 @@ export class VueInterpolationMode implements LanguageMode {
}

findReferences(document: TextDocument, position: Position): Location[] {
if (!_.get(this.config, ['vetur', 'experimental', 'templateInterpolationService'], true)) {
if (!this.interpolationEnabled()) {
return [];
}

Expand Down Expand Up @@ -353,6 +355,13 @@ export class VueInterpolationMode implements LanguageMode {
onDocumentRemoved() {}

dispose() {}

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

Choose a reason for hiding this comment

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

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));
  }

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Back to working through approaches to this, for a follow-up PR. But just as a reminder for myself, the reason is that checkJsDirective is internal: https://github.com/microsoft/TypeScript/blob/master/src/compiler/types.ts#L3010

It's easier to see this in the AST viewer now, thanks to a change that makes internal properties red 🤦‍♂ dsherret/ts-ast-viewer#65

So, looking into additional approaches now...

return !!this.vueInfoService && this.vueInfoService.documentIncludesLanguage(document, 'typescript');
}
}

function getSourceDoc(fileName: string, program: ts.Program): TextDocument {
Expand Down
5 changes: 5 additions & 0 deletions server/src/services/vueInfoService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { TextDocument } from 'vscode-languageserver';
import { getFileFsPath } from '../utils/paths';
import { Definition } from 'vscode-languageserver-types';
import { LanguageModes } from '../embeddedSupport/languageModes';
import { LanguageId } from '../embeddedSupport/embeddedSupport';

/**
* State associated with a specific Vue file
Expand Down Expand Up @@ -82,4 +83,8 @@ export class VueInfoService {
});
return this.vueFileInfo.get(getFileFsPath(doc.uri));
}

documentIncludesLanguage(doc: TextDocument, languageId: LanguageId): boolean {
return this.languageModes.getAllLanguageModeRangesInDocument(doc).some(m => m.languageId === languageId);
}
}
8 changes: 7 additions & 1 deletion test/interpolation/diagnostics/basic.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,13 @@ describe('Should find template-diagnostics in <template> region', () => {
});
});

const noErrorTests: string[] = ['class.vue', 'style.vue', 'hyphen-attrs.vue', 'template-literal.vue'];
const noErrorTests: string[] = [
'class.vue',
'style.vue',
'hyphen-attrs.vue',
'template-literal.vue',
'no-type-check.vue'
];

noErrorTests.forEach(t => {
it(`Shows no template diagnostics error for ${t}`, async () => {
Expand Down
20 changes: 20 additions & 0 deletions test/interpolation/fixture/diagnostics/no-type-check.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<template>
<div>
<div>{{ accessNonexistentProperty }}</div>
</div>
</template>

<script>
export default {
data () {
return {
msg: 'foo'
}
},
computed: {
accessNonexistentProperty() {
return this.message
}
}
}
</script>