atom / language-typescript

TypeScript language package for Atom
Other
21 stars 33 forks source link

`language-typescript` "steals" .js files from `language-babel` when tree-sitting enabled #20

Open awilkins opened 6 years ago

awilkins commented 6 years ago

Prerequisites

Description

When tree-sitting enabled, language-typescript sets the scope of all .js files in projects with a .flowconfig file to flow-javascript.

This overrules the scope for language-babel which is the preferred grammar plugin for Nuclide and breaks all the useful IDE features for Flow.

No idea if this considered a bug but hopefully the presence of this ticket is helpful to other intrepid people who enable new features to test and run into these problems! This puzzled me specifically because I use language-babel and thus have language-javascript disabled, couldn't find the flow-javascript scope string mentioned in any of my installed packages, and didn't think about language-typescript as a thing that would affect JavaScript files, but eventually searched for the scope string on the atom repos and found it.

Steps to Reproduce

  1. Install nuclide and language-babel
  2. Enable tree-sitting in preferences
  3. Open a .js file in a project with a .flowconfig

Expected behavior:

Automatic grammar selection should be "Babel". Useful features like hovering variables to see their type, autocomplete, etc, should work.

Actual behavior:

Grammar selection is "Flow JavaScript". None of the above useful features work.

Reproduces how often:

Always

Versions

Ubuntu 16.04

$ atom-beta --version
Atom    : 1.27.0-beta1
Electron: 1.7.11
Chrome  : 58.0.3029.110
Node    : 7.9.0
awilkins@ryoko:~ 
$ apm-beta --version
apm  1.19.0
npm  3.10.10
node 6.9.5 x64
atom 1.27.0-beta1
python 2.7.12
git 2.7.4

Additional Information

Workarounds

damieng commented 6 years ago

Hey @maxbrunsfeld I think this is because of this file https://github.com/atom/language-typescript/blob/master/grammars/tree-sitter-flow.cson

I don't think a flow config file really belongs in the language-typescript package - in fact I'd say given it takes the same file extension as regular js (.js) and the conflicts that will cause that maybe we shouldn't ship it in core at all?

maxbrunsfeld commented 6 years ago

I think this is largely the same situation that we have with all of the Tree-sitter grammars. They match file extensions that are also matched by TextMate grammars, but they have different scopes. It is intentional, because I think we'd have other problems if we gave new behavior to the existing textmate scopes.

My current thinking is that, at some point, nuclide should update its JavaScript package to include flow-javascript as an additional scope. I'm open to other ideas about how we can make this transition smoother though.

mgtitimoli commented 5 years ago

Hi @maxbrunsfeld,

I understand your point, but I believe in the meantime until that you mentioned happens, this package shouldn't break the other stuff around.

I had a hard time trying to find the origin of this error and I would have never thought it was here, since as @damieng wrote before, this shouldn't belong to this package, or in any case if you thing it should, but by doing that you are breaking some existing stuff, then you should first agree with the other packages into a common direction.

I would suggest to fix this until all the affected packages agree in a common solution to the issue you mentioned.

Thanks.