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

Master Issue for Language Server Crash #136

Closed
octref opened this issue Apr 21, 2017 · 20 comments
Closed

Master Issue for Language Server Crash #136

octref opened this issue Apr 21, 2017 · 20 comments

Comments

@octref
Copy link
Member

octref commented Apr 21, 2017

First, sorry for the LS crashes.

Here is 0.5.6 in case people need to do work: vetur-0.5.6.vsix.zip.
Instruction: https://code.visualstudio.com/docs/editor/extension-gallery#_install-from-a-vsix

I'll be traveling during weekend, so I don't think all crashes can be resolved until Monday.

With that being said, here is a guideline for reporting server crashes to help me better collect info and solve the issue:

Please provide the following info for crashes:

  1. vetur version
  2. error log
  3. a minimal repro case, which
    • should be either a single file or a small project generated by https://github.com/vuejs/vue-cli
    • tweak it until it crashes vetur's LS
    • post the code on GH issue (for single file) or upload your project to GH and post link (for project)

3 is really important, because without a repro I can't do anything.

Also @sandersn, can you take a look at #127 #128 #131 #135? They seem to come from the module resolution code in your PR.

@sandersn
Copy link
Contributor

#128 comes from the hidden, inserted import Vue from 'vue' conflicting with an import Vue written by the user. (Based on my knowledge of the code — I haven't reproed it yet.)

@sandersn
Copy link
Contributor

#127 and #135 seem to be caused by incorrect setup of vetur's dependency on vue-template-compiler. It is likely that I didn't set up this dependency correctly, which is why @MWallin was able to work around the problem by installing vue-template-compiler as part of their own package.

@octref
Copy link
Member Author

octref commented Apr 21, 2017

Yea I also see #128 coming from there. As for #135, @MWallin seems to be using vue-template-compiler as a devDeps and that somehow have an effect on vetur's LS.

@sandersn
Copy link
Contributor

Looks like package.json at root needs to add a dependency on vue-template-compiler according to https://code.visualstudio.com/docs/extensionAPI/extension-manifest

@sandersn
Copy link
Contributor

It's probably as simple as adding

  "dependencies": {
    "vue-template-compiler": "^2.2.1"
  },

in the middle of the root package.json. I'm not sure how to test it though.

@octref
Copy link
Member Author

octref commented Apr 21, 2017

Also, think we should limit the files to do module resolution for limited to only .vue file.

Looks like package.json at root needs to add a dependency on vue-template-compiler according to https://code.visualstudio.com/docs/extensionAPI/extension-manifest

Why so? I'm compiling /server into /client, and then publishing the whole thing. So [email protected] should already be included in the LS.

@sandersn
Copy link
Contributor

I was guessing based on the failing behaviour; it seems like user dependencies on vue-template-compiler (or lack of dependency) affects whether the extension crashes or not. vetur's installation of vue-template-compiler should be independent of whether the user project has a dependency on it. And adding it to the extension manifest (what VS Code calls the root package.json) seems like exactly the sort of extra-npm thing that would make the extension's dependencies separate.

That's my reasoning. It's conjecture but it seems worth testing at least.

I'll contact one of the VS Code guys and see if he can help. (I'll ping him too when I can find his github alias ....)

@sandersn
Copy link
Contributor

sandersn commented Apr 21, 2017

Also, think we should limit the files to do module resolution for limited to only .vue file.

Do you mean that only import X from 'x.vue' should work and not import Y from 'someJavascript'? What about things like import $ from 'jquery' and other common libraries?

@octref
Copy link
Member Author

octref commented Apr 21, 2017

That's my reasoning. It's conjecture but it seems worth testing at least.

I'll do that, package it as vsix and ask people who have problems to try it again.

Do you mean that only import X from 'x.vue' should work and not import Y from 'someJavascript'? What about things like import $ from 'jquery' and other common libraries?

I meant this function https://github.com/octref/vetur/blob/master/server/src/modes/javascriptMode.ts#L89-L109

is even trying to resolve js files like webpack.config.js. As VSCode's js/ts LS already handles that, I think it makes sense if we just resolve modules from .vue files, instead of .vue and .js files. Things like import $ from 'jquery' should still work in script section of any vue file.

@sandersn
Copy link
Contributor

The problem with that code path is that it has no real way to say "I don't handle these kinds of files", so it has to do the fallback itself:

        if (path.isAbsolute(name) || !isVue(name)) {
          return ts.resolveModuleName(name, containingFile, compilerOptions, ts.sys).resolvedModule;
        }
        else {

But ts.resolveModuleName should behave exactly the same way as the default implementation of resolveModuleNames, so there shouldn't be any change in the way that import X from 'webpack.config.js' is resolved. (I'll go look at the code and see if I can confirm that.)

@jonthegiant
Copy link

jonthegiant commented Apr 21, 2017

removed

@octref
Copy link
Member Author

octref commented Apr 21, 2017

@jonthegiant

Can you please open a new issue according to the instruction?

@jonthegiant
Copy link

@octref

Can do, just wasn't sure if this required it's own issue.

@octref
Copy link
Member Author

octref commented Apr 21, 2017

@jonthegiant Haven't seen similar error before so it's worth one.

@octref
Copy link
Member Author

octref commented Apr 21, 2017

@sandersn

import X from 'webpack.config.js' is resolved.

I meant the containingFile here only be .vue files.

resolveModuleNames(moduleNames: string[], containingFile: string): ts.ResolvedModule[] {

Requiring .js files in .vue files should certainly be supported.
But I don't see the use of, say, resolve the require('path') in a user's webpack.config.js.
I'm mainly worried since there are reports about CPU and ram usage #127 (comment) and #131 and my guess is they come from trying to do too much module resolution in large projects.

@sandersn
Copy link
Contributor

Oh, I see. A user might have giantFile.js open in one buffer, which is using VS Code's instance of the TS language service. Then when they do import X from 'giantFile' in a .vue file, Vetur's instance of the TS language service will go off and also analyse giantFile.js.

@sandersn
Copy link
Contributor

I think there may be some tsconfig settings to limit how deeply the compiler follows module resolution inside node_modules.

@MWallin
Copy link

MWallin commented Apr 21, 2017

Don't know if its relevant or not, but I didn't really install vue-template-compiler myself. It was a part of the webpack-simple CLI template. Might that perhaps be part of why its hard to isolate whats wrong, lots of different ways to setup your projects...

@exonuclease
Copy link

When i open the project folder contained node_modeules the error message is JavaScript heap out of memory and if i open the components folder the error message is start < 0.it is strange

@octref
Copy link
Member Author

octref commented Apr 25, 2017

start < 0 should be fixed with 0.6.2. Heap problem seems unresolved yet -- see #131.

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

No branches or pull requests

5 participants