search

atom / first-mate

TextMate helpers
http://atom.github.io/first-mate
MIT License
91 stars 57 forks source link

Loading grammar with highlights throws missing scopeName property error #106

Open austinkelleher opened 6 years ago

austinkelleher commented 6 years ago

Description

We are using the language-javascript package (along with several others) in combination with highlights to syntax highlight a REPL on a webpage. It seems that sometime recently new grammar files were added without a scopeName property (e.g. https://github.com/atom/language-javascript/blob/master/grammars/tree-sitter-javascript.cson) and instead are using legacyScopeName. This is causing an error to be thrown from first-mate as it is expecting a scopeName property. I'm not sure what the right solution is to this. We could either:

  1. Add a scopeName property to each of the grammar files that is missing it
  2. Add support for legacyScopeName in this module

I'm leaning towards 2 as the better solution, but I'm not really sure why legacyScopeName was introduced in the first place. I would be happy to work on this and submit a PR given proper feedback on the above information.

Case:

const Highlights = require("highlights");

const highlighter = new Highlights();

highlighter.requireGrammarsSync({
  modulePath: require.resolve("language-javascript/package.json")
});

Throws the following error:

Caused by: Error: Grammar missing required scopeName property: /node_modules/language-javascript/grammars/tree-sitter-javascript.cson
  at GrammarRegistry.module.exports.GrammarRegistry.readGrammarSync (/node_modules/first-mate/lib/grammar-registry.js:107:15)
  at GrammarRegistry.module.exports.GrammarRegistry.loadGrammarSync (/node_modules/first-mate/lib/grammar-registry.js:133:22)
  at Highlights.module.exports.Highlights.requireGrammarsSync (/node_modules/highlights/lib/highlights.js:95:25)

/cc @50Wliu @maxbrunsfeld

50Wliu commented 6 years ago

Tracking in atom/atom#16581.

50Wliu commented 6 years ago

Actually that is related but not quite the same.

austinkelleher commented 6 years ago

Do have any thoughts on the two potential fixes I suggested @50Wliu?

50Wliu commented 6 years ago

I will defer to @maxbrunsfeld who is working on tree-sitter.

austinkelleher commented 6 years ago

@maxbrunsfeld Any thoughts on this? This bug is currently breaking our REPL for: https://markojs.com/try-online/

austinkelleher commented 6 years ago

Ping @maxbrunsfeld @50Wliu following up with this issue since there is still no consensus. Thoughts on my suggested fixes? https://github.com/atom/first-mate/issues/106#issue-290073017