reasonml-editor / vscode-reasonml

OCaml & Reason support for Visual Studio Code
Apache License 2.0
491 stars 62 forks source link

VSCode upgrade broke extension/merlin support in WSL #186

Closed superherointj closed 6 years ago

superherointj commented 6 years ago

Since VSCode (& extension?) was upgraded it now errors in WSL. Configuration is the same (that used to work).

app re - assets - visual studio code

superherointj commented 6 years ago

Discord's "ssrb" said:

it seems the user settings are no longer applied on top of the default
if I hardcode the ocamlmerlin cmd in the extension (merlin.js) it works.

initialize() { return __awaiter(this, void 0, void 0, function* () { //const ocamlmerlin = this.session.settings.reason.path.ocamlmerlin; const ocamlmerlin = "bash -ic ocamlmerlin";

I reckon onDidChangeConfiguration/LSP.DidChangeConfigurationParams could be the culprit. The ocaml language server no longer gets the settings. Maybe they changed the behavior ?

raiscui commented 6 years ago

image

maybe is same?

ghost commented 6 years ago

It will probably be a few days before I have a chance to look into this but in the meantime if anyone wants to try the previous versions you can do the following:

npm install -g vsce
git clone https://github.com/reasonml-editor/vscode-reasonml
cd vscode-reasonml
git checkout tags/v1.0.26
yarn install
yarn run build
vsce package .
code --install-extension reasonml-v1.0.26.vsix
siggirh commented 6 years ago

I have no experience with creating vscode extensions but it seems like the initialize method is run twice. Is that normal? First it's run with reason.path.ocamlmerlin set as ocamlmerlin (not my setting) and then it's run with the correct settings parameter bash -ic ocamlmerlin. However the second initialize() doesn't run if the first call crashes (where ocamlmerlin is not recognized as a command).

If I hardcode the variable as per @superherointj 's quote above, the extension indeed works but it's working because the reason.path.ocamlmerlin variable is not being read incorrectly as ocamlmerlin on the first call to initialize(). Then it doesn't crash and both calls to initialize() work as expected.

I attached a screenshot to show what I mean. The top window is me editing the extension code, bottom is another window running a reason project.

stuff

No idea if this helps but it's what I've found so far in limited time looking at it :)

siggirh commented 6 years ago

The problem seems to be the settings merge in the ocaml-language-server. The deepmerge params are in the wrong order. According to deepmerge:

merge(x, y, [options]) Merge two objects x and y deeply, returning a new merged object with the elements from both x and y. If an element at the same key is present for both x and y, the value from y will appear in the result.

In ocaml-language-server/lib/index.js:

function withDefaults(overrides) {
  return deepmerge(overrides || {}, ISettings.defaults);
}

So the defaults always win.

However just swapping the params doesn't seem to be enough. The overrides object doesn't have the reason top-level key so the merged data will consist of both objects, like so:

{ codelens: { unicode: true, enabled: true },
  debounce: { linter: 500 },
  diagnostics: { tools: [ 'bsb', 'merlin' ] },
  format: { width: 100 },
  path: 
   { bsb: 'bash -ic bsb',
     ocamlfind: 'bash -ic ocamlfind',
     esy: 'esy',
     env: 'env',
     ocamlmerlin: 'bash -ic ocamlmerlin',
     opam: 'bash -ic opam',
     rebuild: 'bash -ic rebuild',
     refmt: 'bash -ic refmt',
     refmterr: 'bash -ic refmterr',
     rtop: 'bash -ic rtop' },
  server: { languages: [ 'ocaml', 'reason' ] },
  reason: 
   { codelens: { enabled: true, unicode: true },
     debounce: { linter: 500 },
     diagnostics: { merlinPerfLogging: false, tools: [Object] },
     format: { width: null },
     path: 
      { bsb: 'bsb',
        env: 'env',
        esy: 'esy',
        ocamlfind: 'ocamlfind',
        ocamlmerlin: 'ocamlmerlin',
        opam: 'opam',
        rebuild: 'rebuild',
        refmt: 'refmt',
        refmterr: 'refmterr',
        rtop: 'rtop' },
     server: { languages: [Object] } } }
ghost commented 6 years ago

I just made some further changes and published a new version 1.0.31 and I think this should actually be fixed now.