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

Add vue.d.ts and typescript support #94

Merged
merged 13 commits into from Mar 21, 2017

Conversation

sandersn
Copy link
Contributor

@sandersn sandersn commented Mar 16, 2017

  1. Add an implicit import Vue from 'vue' and new Vue(...) around the default exported object literal. This provides a lot better intellisense than just a bare object literal because the Typescript language server knows that the object will be used by Vue.
  2. Resolve other .vue files so that import other from './other.vue' provides correct info.
  3. If lang='typescript' is specified, put the TS language service in TS mode: all errors are reported and completions include only symbols that are known to be correct.
  4. Also fix a bug in htmlServerMain so that errors are actually reported. Looks like this is already fixed in master.

Fixes #75

Probably it should replace the space-replacing
code for individual regions with a one-shot parsing that gets looked up
for each mode as updateCurrentTextDocument is called.
1. Add an implicit `import Vue from 'vue'` and `new Vue(...)` around the
default exported object literal. This provides a lot better intellisense
than just a bare object literal because the Typescript language server
knows that the object will be used by Vue.
2. Resolve other .vue files so that `import other from './other.vue'`
provides correct completions.
3. If lang='typescript' is specified, put the TS language service in TS
mode: all errors are reported and completions include only symbols that
are known to be correct.
4. Also fix a bug in htmlServerMain so that errors are actually reported.
@sandersn
Copy link
Contributor Author

@mhegazy you were interested in this PR, you said.

@octref
Copy link
Member

octref commented Mar 16, 2017

Thanks a lot for the PR!

Just trying it out on this repo: https://github.com/vuejs/vue-hackernews-2.0, but IntelliSense didn't work:

  • Vue. doesn't prompt Vue.extend in *.vue files (works in js files). Thought this would work with the implicit addition of import Vue from 'vue'
  • Newly opened *.vue files don't have IntelliSense and always show "loading"
    image
  • As for new Vue(...), if I have Item.vue with a prop count, and in another vue file I import Item.vue, is it that
    • Item. is supposed to prompt all values on the new Vue(...) object?
    • Item.props. should prompt count?

Just a rough look at the code, it seems I need a properly configured jsconfig/tsconfig.

Would you mind adding appropriate config for a sample vue project so it would work as you intended? Either https://github.com/vuejs/vue-hackernews-2.0 or something from here https://github.com/vuejs/vue/tree/dev/examples

@octref
Copy link
Member

octref commented Mar 16, 2017

Oh and in vetur's language server I'm using TS 2.2.1. Is is I should install a dev build or build from typescript's master and link locally?

@sandersn
Copy link
Contributor Author

I'll test with vue-hackernews-2.0, in particular without a tsconfig. I thought the change would work without a tsconfig, but I didn't test that way, so I'm not surprised that it doesn't.

  • I don't think you need any special version of TS. I'll double check though.
  • "Loading..." usually means the language service asserted and hit the debugger statement we have there. I'll check it out. (This is probably the cause of no completion for Vue.extends too.)
  • Is this the code you're thinking of?:
// @filename:Item.vue
export default {
  props: ['count'],
  data: {
    d1: 12
  }
  // etc ...
}

// @filename:SomeComponent.vue
//          (or somethingElse.js)
import Item from './Item.vue'
Item./**/ // should see d1, $data, $parent, etc
Item.props./**/ // should see count

The short answer is yes, those should work. But vue.d.ts doesn't support a lot of these things yet. @DanielRosenwasser is working on improvements to vue.d.ts that rely on Typescript 2.3 features. Once 2.3 ships with VS Code, the experience will get a lot better.

Unfortunately, better this inference relies on 2.3 as well. Even without 2.3, though, this change makes module resolution work, and will pick up any other improvements to vue.d.ts that happen in the meantime.

@octref
Copy link
Member

octref commented Mar 16, 2017

I tested with lodash and @types/lodash, module resolution works great. Thanks!

Is this the code you're thinking of?

Something like that. I'm wondering if there is a way to get the Vue wrapper object's type information on Item.vue at LS side so I can use it to power html suggestion. Item./**/ IntelliSense in JS region wouldn't be helpful. But this kind of IntelliSense in html template region would be godly:

<i/**/ // Prompt item

<item :/**/> // Prompt count and other props

@sandersn
Copy link
Contributor Author

Yes, from typescript's perspective the object from Item.vue is already a Vue object because it was called from new Vue(...). However, you would have to make the typescriptMode.parse code much smarter to use that in HTML. Currently it just replaces things outside <script> with spaces. If it replaced non-standard HTML elements with valid typescript, which would each have to be smaller than the original HTML so they didn't overflow the original, it could provide completions on that typescript.

I think the Angular guys have done or were thinking about doing something like that for Angular 2. I don't know Angular though, so I'm not sure of the details.

You might also do some other clever thing with a virtual TextDocument for each HTML element, which would make the host code in javascriptMode much more complicated. And I don't know whether you could do anything like validation, only completions, because the offsets from the language service would likely be quite wrong.

@sandersn
Copy link
Contributor Author

One update and one question:

  1. I confirmed that this works with Typescript on master and 2.2. So both typescript@next and the current Typescript should be fine.
  2. Should the inserted call be Vue.extends instead of new Vue?

@octref
Copy link
Member

octref commented Mar 16, 2017

Should the inserted call be Vue.extends instead of new Vue?

I think new Vue.

// (the span of the construct call is the same as the object literal)
const objectLiteral = (exportDefaultObject as ts.ExportAssignment).expression as ts.ObjectLiteralExpression;
const o = <T extends ts.TextRange>(n: T) => ts.setTextRange(n, objectLiteral);
const vue = ts.setTextRange(ts.createIdentifier('Vue'), { pos: objectLiteral.pos, end: objectLiteral.pos + 1 });
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind giving o and n more descriptive names? I'm not quite familiar with TS compilers but would want to fiddle with this part of code.

@octref
Copy link
Member

octref commented Mar 17, 2017

I'll merge once you fix the one comment.

Meanwhile thanks for giving me a starting point -- I guess for now I'll just use TS to parse the object literal's AST, find out props / methods / data manually to provide some IntelliSense.

image

It's required to make sure that getXXXAtLocation works correctly,
because if the ranges are missing from the synthetic nodes, the language
service asserts, and if the ranges are wrong, the language service's AST
search gets confused.
@sandersn
Copy link
Contributor Author

I improved the naming, structure and documentation of the AST creation utilities. Let me know if you have other questions about creating or altering ASTs.

I'm still investigating the failure to resolve 'vue'. It works fine when I use './node_modules/vue/types/index' instead. My test project may have incorrect settings in tsconfig.

return vueDocument.getEmbeddedDocument('typescript');
}
return vueDocument.getEmbeddedDocument('javascript');
});

let compilerOptions: ts.CompilerOptions = { allowNonTsExtensions: true, allowJs: true, lib: ['lib.es6.d.ts'], target: ts.ScriptTarget.Latest, moduleResolution: ts.ModuleResolutionKind.Classic };
Copy link

Choose a reason for hiding this comment

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

For this i would defere to ts.getDefaultCompilerOptions and just set:

let compilerOptions = ts.getDefaultCompilerOptions();
if (docs[fileName].languageId !== 'typescript') {
     copilerOptions.allowJs = true
}
compilerOptions.allowNonTsExtensions = true;

Copy link

Choose a reason for hiding this comment

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

I take that back..

{
   "allowJs": docs[fileName].languageId !== 'typescript',
   "lib" : ["dom", "esNext"],
   "target" : ["esNext"],
   "module": ts.ModuleKind.CommonJs
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"esnext" didn't ship as a lib target in 2.2, so I'll use "es2017" for now.
(It was committed Dec 30, but I guess it was part of a PR that got merged later.)

compilerOptions,
configFile,
undefined,
[{ extension: 'vue', isMixedContent: true }]).fileNames;
Copy link

Choose a reason for hiding this comment

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

should also set compilerOptions to the options from the file. we also need to set compilerOptions.allowNonTsExtensions = true; all the time to allow .vue files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

export function createUpdater() {
const clssf = ts.createLanguageServiceSourceFile;
Copy link

Choose a reason for hiding this comment

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

these should be added to the LanguageServiceHost API in TS 2.3, I would also add a comment here with that to allow removing this global function hijacking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comment where the hijacking happens. This part of the code may remain the same since it's basically the same way that the plugin API allows users to wrap the language service.

}
return '1'; // default lib an jquery.d.ts are static
else {
return (ts as any).getScriptKindFromFileName(fileName);
Copy link

Choose a reason for hiding this comment

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

We need to expose this on the API for TS 2.3, also add a comment allowing removal of this cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comment. I'll send out a PR for the API change after this.


let compilerOptions: ts.CompilerOptions = { allowNonTsExtensions: true, allowJs: true, lib: ['lib.es6.d.ts'], target: ts.ScriptTarget.Latest, moduleResolution: ts.ModuleResolutionKind.Classic };
let currentTextDocument: TextDocument;
let scriptFileVersion: number = 0;
let versions: ts.MapLike<number> = {};
Copy link

Choose a reason for hiding this comment

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

you can use ES6 Map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@sandersn
Copy link
Contributor Author

All right, I addressed all of @mhegazy's comments. I think it's ready to go in but I'll spend time testing without a tsconfig now to make sure.

@sandersn
Copy link
Contributor Author

The failure to resolve import Vue from 'vue' was due to missing allowSyntheticDefaultImports: true and having moduleResolution: ts.ModuleResolutionKind.Classic. It needs to be NodeJs.

@sandersn
Copy link
Contributor Author

OK, vue-hackernews-2.0 is now working and gives nice (?) errors when you switch to lang="typescript". Actually the vue.d.ts needs some updates to make this mode actually nice because it issues some bogus errors.

For both javascript and typescript, the completions are pretty nice, though.

I'm chasing down some bugs to do with changing the language back and forth between javascript and typescript. I think this is ready to merge as-is though. I'll send another PR if/when I track down the language-changing bug.

@sandersn
Copy link
Contributor Author

Actually, the language-change bug repros when opening or closing files. Probably you should wait to merge this until I track it down.

@sandersn
Copy link
Contributor Author

If you open a .vue file with lang='typescript' and then open one without, then the newly opened file gets initially set as typescript, even though it shouldn't. But then when you interact with the language service, it resets the type to javascript, which hits the assert.

@sandersn
Copy link
Contributor Author

vue.d.ts is the one that needs to use ThisType I think; I'm not sure what vetur would need to do. @DanielRosenwasser is working on an update right now.

I have the bug almost tracked down -- vue-hackernews works great as long as you don't change one of the files to typescript and then try to open other files :)

@sandersn
Copy link
Contributor Author

Hm, so this is not an easy fix. The problem is that javascriptmode.ts doesn't know the languageId of files that are not opened yet, but the language service still wants to know whether they contain JavaScript or typescript. There are two basic approaches to fix this:

  1. Start with an initial guess. Right now this causes an assert if the guess is wrong because the file type switches in the language service.
  2. Add files to the language model cache as the language service requests them. This bloats the cache, but arguably it makes vetur itself more like a real language service.

I think (2) is the right solution, but it's more work. I'll do what I can but my next 48 hours are pretty busy with non-work things, so it may be Monday before I have something working.

The Typescript language service references files that are not open for
editing, and thus don't have entries in the language model cache. Since
they don't have entries, there is no way to know whether their script
block is Javascript or Typescript.

The fix is to load these files and register them with the language model cache.
@sandersn
Copy link
Contributor Author

sandersn commented Mar 18, 2017

Turns out the fix is pretty small after all: calling jsDocuments.get on a new TextDocument implicitly registers it with the cache.

Unfortunately, since I was working from my laptop, a Windows machine, I discovered that path handling is completely broken on Windows. url.parse(filename).path is not a complete solution for Windows filenames, which are rooted at c:/ or some other drive. There's an extra preceding / and (at least) the ':' is encoded as '%3A'. This has to be a solved problem within VS Code, so I'm going to find out what the existing solution is.

@octref
Copy link
Member

octref commented Mar 20, 2017

Are you looking for this?

https://github.com/Microsoft/vscode/blob/master/src/vs/base/common/uri.ts#L126-L145

I'm not against including that as part of the source. I might need it too.

However, overall I think it's better to have a fine-tuned default ts/js config internally, as I imagine most Vue users wouldn't bother to specify js/ts config. (Though reading tsconfig if it's present doesn't hurt).

@sandersn
Copy link
Contributor Author

Yes, I am also looking at it right now and wondering what the public version of it is. I asked @mjbvz as well so I should know pretty soon.

Unfortunately, the host also calls ts.sys.readFile several times so that the language service is able to analyse files that are not open in the editor. It's not just for tsconfig support. 😿

@sandersn
Copy link
Contributor Author

Looks this package is the right answer: https://github.com/Microsoft/vscode-uri

TS server and VS Code normalise them in different ways. Standardise by
passing everything through vscode-uri
@sandersn
Copy link
Contributor Author

Ok, windows paths should be working now. Both URL encoding and slash normalisation had to be fixed. I'll check once more on my Linux box to make sure I didn't break anything there, but I think it's ready to merge now.

@sandersn
Copy link
Contributor Author

All right, everything looks fine on Linux as well, so this PR is good to go.

@octref octref merged commit c6bbd43 into vuejs:master Mar 21, 2017
@octref
Copy link
Member

octref commented Mar 21, 2017

Thanks a lot! Will include this in 0.6 release.

@octref
Copy link
Member

octref commented May 8, 2017

@sandersn Do you know @DanielRosenwasser's progress of the vue.d.ts update that would use contextualThis to transform the wrapped new Vue() object?

Or is is I should just start writing a dts lib that would transform a new Vue() object into an extended form which would have all props, methods and computed bound to it? Then I can put it in here to bind the new object to this: https://github.com/octref/vetur/blob/master/server/src/modes/typescriptMode.ts#L68-L74

@DanielRosenwasser
Copy link
Contributor

Right now you can try it all out on https://github.com/DanielRosenwasser/typescript-vue-todomvc, but I need to chat with @yyx990803 about it, but I think the earliest we could get it out is Vue 2.4.

By the way, right now TypeScript users can't rely on contextually typing the object literal because at compile time, there is no call to Vue.extend (or new Vue). So while you'll get no errors in JS scenarios, TypeScript will spit out errors when you run Webpack.

In short, TypeScript users still need to wrap their components in calls to Vue.extend which I think is fine.

@octref
Copy link
Member

octref commented May 8, 2017

Right now you can try it all out on https://github.com/DanielRosenwasser/typescript-vue-todomvc

Working great for me, thanks!

TypeScript users still need to wrap their components in calls to Vue.extend

That's fine. I'm wondering if it's possible to modify the source at compile time using a TS loader for webpack.
I didn't delve into vue's internal yet, but I guess Vue.extend is the same thing as class App extends Vue? People are already writing

export default class App extends Vue {
  ...
}

with https://github.com/vuejs/vue-class-component so I guess that's fine.

Also wondering what's your thoughts on typed props. I see some work towards that in vue.d.ts.

Meanwhile I'll start building support for dynamic IntelliSense for template completion in #145. That seems doable without contextual typing anyway.

@DanielRosenwasser
Copy link
Contributor

Also wondering what's your thoughts on typed props

Props is pretty crazy to type because it might come in as an array of strings, or it might come in as an object which you can infer from. Ideally I could have a PropNames extends string = never and Props, and have props: SomeMappedType<Props> | PropNames[] (or something like that), but it started getting complicated.

I'll try it out after next week. Right now we're getting some stuff together for our Build conference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better intellisense for this references in script block
4 participants