merge vue-language-server #227

HerringtonDarkholme · 2017-06-08T02:20:30Z

This is a huge pull request.

Main idea:

change server into a separate npm module. So debug/release can be independent of client.
change client to depend on npm module. This keeps server code in same repo and issues concentrated. Development does not need installServerIntoExtension but a npm link

Other things include code improvement, stylus support and test code.

I think it might also be an option to create a repository for vetur in vuejs organization.

mickdekkers · 2017-06-08T07:52:37Z

If this is to be accepted, may I suggest using lerna? It would make it easier to add and manage additional packages in the repo in the future.

HerringtonDarkholme · 2017-06-08T07:54:50Z

It only have two packages. learna might be an overkill, I think.

octref · 2017-06-11T21:57:17Z

server/src/htmlServerMain.ts

-  Disposable,
-  DocumentSelector
-} from 'vscode-languageserver';
+import { createConnection, TextDocuments, InitializeParams, InitializeResult, DocumentRangeFormattingRequest, Disposable, DocumentSelector } from 'vscode-languageserver';


Maybe name this as vueServerMain?

octref · 2017-06-11T21:58:27Z

server/src/htmlServerMain.ts

  'vue-html': true,
  html: true,
  css: true,
  scss: true,
  less: true,
-  javascript: true
+  javascript: true,


I'm against this styling, but if we were to do it I'd add it to the tslint rule.

Let's add tslint rule after this pull request, definitely need it.

octref · 2017-06-11T21:59:53Z

server/src/htmlServerMain.ts

+  if (mode && mode.doComplete) {
+    return mode.doComplete(document, textDocumentPosition.position);
+  } else {
+    return getVls().doVueComplete();


0e56992 and #219.

octref · 2017-06-11T22:01:31Z

server/src/modes/embeddedSupport.ts

  let languageIdFromType = removeQuotes(lang);
  if (languageIdFromType === 'jade') {
    languageIdFromType = 'pug';
-  }
-  if (languageIdFromType === 'ts') {
+  } if (languageIdFromType === 'ts') {


For non if-else, I'd put if on another line for better visibility.

octref · 2017-06-11T22:03:41Z

server/src/modes/languageModes.ts

-  let modes = {
-    vue: getVueMode(),
+  let jsMode = getJavascriptMode(documentRegions, workspacePath)
+  let modes: {[k: string]: LanguageMode} = {


Maybe put 'lang instead of k for key.

octref · 2017-06-11T22:08:25Z

server/src/modes/template/parser/htmlScanner.ts

@@ -194,7 +203,7 @@ export interface Scanner {
  getScannerState(): ScannerState;
 }

-const htmlScriptContents = {
+const htmlScriptContents: {[k: string]: boolean} = {


octref · 2017-06-11T22:09:21Z

server/src/modes/template/services/htmlHover.ts

+        hover = { contents, range };
+      })
+    }
+    return hover


octref · 2017-06-11T22:09:45Z

server/src/modes/template/services/htmlHover.ts

+      return getAttributeHover(node.tag, attribute, tagRange)
+  }
+
+  return NULL_HOVER


octref · 2017-06-11T22:14:37Z

server/src/modes/utils/path.ts

-  } else {
-    return Uri.parse(documentUri).path;
-  }
-}


You are still using this in js/script mode, right?

Just moved to script mode.

octref · 2017-06-11T22:16:43Z

server/src/utils/strings.ts

@@ -21,12 +21,25 @@ export function getWordAtText(text: string, offset: number, wordDefinition: RegE
  return { start: offset, length: 0 };
 }

+export function repeat(value: string, count: number) {


Since lodash is a dep I'd just use _.repeat, 6b2c8ce

octref · 2017-06-11T22:25:05Z

Overall comments:

Good job on bringing back test and the stylus support.
I think it's better to stick with yarn, since npm5 is buggy and not many people are using it.
There are some styling / consistency problems. If we are to do this together, what do you think about using prettier with a line width limit like 120 or 140? I'm using this plugin: https://github.com/esbenp/prettier-vscode
I haven't been able to build yet, since I'm having really slow connection now and not npm5 yet. I just took a general look but haven't looked some of the implementations more carefully.
Overall I'm pro for merging this asap and we can address bugs later. Otherwise I don't want to push too much changes to master. Once this settles down I can start adding some work for Type checking for template expressions #209.

Again, thanks a lot for the PR 👍

octref · 2017-06-11T22:25:40Z

And BTW, would you be OK if I push some changes to your branch?

octref · 2017-06-13T05:24:58Z

server/src/modes/script/javascript.ts

      if (!languageServiceIncludesFile(jsLanguageService, doc.uri)) {
-        return [];
+        return []
      }



Can we not remove semicolons randomly...

I have consistently remove semicolons for new code.

The real problem is whether we need semicolons

Maybe we can agree on running it through prettier with printWidth 120?

octref · 2017-06-13T05:29:44Z

server/src/modes/script/bridge.ts

+import Vue from 'vue'
+export default function test<T>(t: T & Vue.ComponentOptions<{}>): T {
+  return t
+}`


What is this for?

export default function test<T>(t: T & Vue.ComponentOptions<{}>): T { return t }

Also, I'd like to not put too many things into this PR. Let's fix the existing problems and merge it.

This is important fix for resolving component info.

Note new Vue or Vue.extend will erase component definition info. But an identity function like before will keep component definition object's type. So we can resolve props/components.

Sounds good, but maybe also update the comments on the TS mode file.

octref · 2017-06-13T05:49:25Z

client/src/htmlMain.ts

@@ -11,7 +11,7 @@ namespace ColorSymbolRequest {
 export function activate (context: ExtensionContext) {

  // The server is implemented in node
-  const serverModule = context.asAbsolutePath(path.join('client', 'server', 'vueServerMain.js'));
+  const serverModule = require.resolve('vue-langauge-server');


langauge -> language

octref · 2017-06-13T06:14:24Z

I pushed a change to master to make sure tslint works with tslint@5.
Can you download the tslint plugin and fix your styling issues.
Otherwise looks good, thanks!

octref · 2017-06-13T06:46:17Z

server/src/modes/script/typescript.ts

+      setZeroPos(ts.createLiteral('vue-editor-bridge'))));
+    sourceFile.statements.unshift(vueImport);
+
+    // 2. find the export default and wrap it in `new Vue(...)` if it exists and is an object literal


Then maybe update the description here too.

HerringtonDarkholme · 2017-06-13T11:29:47Z

Fixes have been pushed.

HerringtonDarkholme · 2017-06-13T15:17:27Z

One thing left is updating client folder's yarn lock. My yarn is configured to taobao's registry, not yarn-pkg. (Curse GFW).

HerringtonDarkholme · 2017-06-15T01:41:08Z

@octref Ping

HerringtonDarkholme · 2017-06-15T06:17:42Z

Thanks!

merge vue-language-server

merge vue-language-server

3f500eb

octref requested changes Jun 11, 2017

View reviewed changes

HerringtonDarkholme added 4 commits June 12, 2017 10:40

update stylus

f058b47

rename main file

81b911b

address code review feedback

893b83f

add router tag completion

0eabea2

octref requested changes Jun 13, 2017

View reviewed changes

octref reviewed Jun 13, 2017

View reviewed changes

HerringtonDarkholme added 4 commits June 13, 2017 19:18

Merge branch 'master' of github.com:octref/vetur

f775b8d

tslint fix

513e66b

fix style rule

3aa26cb

add necessary comment and fix typo

c75e767

octref approved these changes Jun 15, 2017

View reviewed changes

octref merged commit f812f40 into vuejs:master Jun 15, 2017

octref mentioned this pull request Jun 19, 2017

vetur 1.0 #188

Closed

12 tasks

octref added a commit that referenced this pull request Nov 22, 2017

Merge pull request #227 from HerringtonDarkholme/master

c2075c9

merge vue-language-server

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

merge vue-language-server #227

merge vue-language-server #227

HerringtonDarkholme commented Jun 8, 2017

mickdekkers commented Jun 8, 2017

HerringtonDarkholme commented Jun 8, 2017

octref Jun 11, 2017

HerringtonDarkholme Jun 12, 2017

octref Jun 11, 2017

HerringtonDarkholme Jun 12, 2017

octref Jun 11, 2017

octref Jun 11, 2017

octref Jun 11, 2017

octref Jun 11, 2017

octref Jun 11, 2017

octref Jun 11, 2017

octref Jun 11, 2017

HerringtonDarkholme Jun 12, 2017

octref Jun 11, 2017

octref commented Jun 11, 2017

octref commented Jun 11, 2017

octref Jun 13, 2017

HerringtonDarkholme Jun 13, 2017

octref Jun 13, 2017

HerringtonDarkholme Jun 13, 2017

octref Jun 13, 2017

HerringtonDarkholme Jun 13, 2017

octref Jun 13, 2017

octref Jun 13, 2017

octref commented Jun 13, 2017

octref Jun 13, 2017

HerringtonDarkholme commented Jun 13, 2017

HerringtonDarkholme commented Jun 13, 2017

HerringtonDarkholme commented Jun 15, 2017

HerringtonDarkholme commented Jun 15, 2017

merge vue-language-server #227

merge vue-language-server #227

Conversation

HerringtonDarkholme commented Jun 8, 2017

mickdekkers commented Jun 8, 2017

HerringtonDarkholme commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octref commented Jun 11, 2017

octref commented Jun 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octref commented Jun 13, 2017

Choose a reason for hiding this comment

HerringtonDarkholme commented Jun 13, 2017

HerringtonDarkholme commented Jun 13, 2017

HerringtonDarkholme commented Jun 15, 2017

HerringtonDarkholme commented Jun 15, 2017