Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

[vscode] Use local path for bsb and refmt #210

Merged
2 commits merged into from
May 23, 2018

Conversation

jchavarri
Copy link
Member

@jchavarri jchavarri commented May 14, 2018

Fixes #203.

Companion of https://github.com/freebroccolo/ocaml-language-server/pull/120.

Tested on macOS.

This PR changes a couple of things:

  • reason.path.bsb uses the exe format that is around 1 order of magnitude faster for small projects (cc @bobzhang)
  • reason.path.refmt and reason.path.bsb point to the bs-platform local binaries so any incompatible updates don't break existing projects (plus, both tools are kept in sync).

The users of WSL in Windows shouldn't be affected as they already have to override this flag to make WSL work.

The main downside is that it breaks a potential usage in native Windows, but I believe there are other more critical things to solve on that front first, so we can revisit when it's ready.

@ghost ghost merged commit b05a9c9 into reasonml-editor:master May 23, 2018
@bsansouci
Copy link

What's the potential breakage that this causes? Is it because it's a unix-like path?

@jchavarri
Copy link
Member Author

@bsansouci Yeah, essentially that.

@bsansouci
Copy link

If this path is used by the vscode extension itself, could we support unix paths for that specific field?

@jchavarri
Copy link
Member Author

I'm not sure what you mean... the path is already unix-friendly?

@bsansouci
Copy link

I may be confused, but if the editor plugin reads this field to figure out what to call under the hood, we can make the plugin understand unix path even on windows. This would make the extension work again on windows.

@jchavarri
Copy link
Member Author

jchavarri commented May 23, 2018

Oh, I think I see what you mean. Replacing the path at runtime in the lang server, to adapt it to Windows paths, right? Yeah, maybe we could do that... One potential concern would be that we'd need to make sure we know the user is running bsb from Windows, or rather from WSL, as making the wrong checks could break legit uses where the paths have to be unix friendly and we'd be replacing them wrongly. Does that make sense? I'm not very familiar with Windows, so take what I say with a grain of salt.

Also @freebroccolo what you you think?

This would make the extension work again on windows.

If someone is using bsb through the VSCode extension in Windows, they must have replaced the reason.path.bsb in their VSCode settings, so changing the default value shouldn't affect them (so this PR should be safe). Have you seen any breakages in Windows?

@bsansouci
Copy link

Yeah that's right. We need to make sure it works for WSL and cmd.exe.

I haven't seen ay breakage but it seems like if anyone's using the extension without having this field replaced, their setup would break right?

@jchavarri
Copy link
Member Author

Oh, does BuckleScript work on Windows natively? From the official docs and the thread in reasonml/reasonml.github.io#195 I always thought Windows users always had to go through configuring WSL (which involves overriding that setting).

If there are users that are running bsb from the VSCode extension in Windows native without that setting, yes, their setup would break. I just thought that was not a possibility 😕

@bsansouci
Copy link

Yes absolutely, Bucklescript fully works on cmd.exe. It's a good example to follow hehe.
So we should fix this. How do people deal with cross platform paths in editors plugins like this?
If windows paths can't contain / then we could just split on that to check...

@jchavarri
Copy link
Member Author

One idea that I like is to avoid using runtime code to differ which platform the server is running etc and set specific defaults for each platform, like shown in microsoft/vscode#22829 (comment).

There's an example implementation in vscode-ocaml:

https://github.com/hackwaly/vscode-ocaml/blob/85483d4311846285e30e71cf18cb2f5a1eaf9371/src/extension.ts#L459-L471

That way, we can provide sane defaults for each platform+setting. The downside is having more settings.

The alternative is to leave just one setting, and change that path at runtime.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants