Skip to content
This repository was archived by the owner on Apr 1, 2020. It is now read-only.

Fix tsx jsx grammar loading #1047

Merged
merged 1 commit into from
Dec 1, 2017
Merged

Conversation

akinsho
Copy link
Member

@akinsho akinsho commented Nov 30, 2017

@bryphe, excellent work on the new Syntax highlighting (I know its still a [WIP] but looks great so far). I noticed while trying it though that it wasn't picking up javascript.jsx and typescript.tsx as tsx and jsx filetypes (which is what I believe most viml tsx and jsx plugins use or rather what they coerce the filetype to). The default config appears to be trying to account for these filetypes I think, but I noticed I wasn't getting the grammar loaded.

I Tweaked the grammar loader to check if a filetype contains a dot like javascript.jsx in which case I split the value and return the first half so the loader can accurately use the js or ts textmate objects.

I wasn't entirely sure if you'd want this sort of thing to be user configured(so this is a question/PR), although it took a tiny bit of digging to find out what was going on, If you rather this were user configured I could make an entry in the docs instead.

Sidenote: I've seen/used a plugin (not very popular) that set the ft to typescript.jsx which wouldn't be accounted.

javascript.jsx and typescript.tsx are very common filetypes
set by vim typescript and js plugins
tweaked the grammar config checker to pickup these filetypes
@bryphe
Copy link
Member

bryphe commented Nov 30, 2017

@bryphe, excellent work on the new Syntax highlighting (I know its still a [WIP] but looks great so far).

Cheers, thanks @Akin909 😄 Still some issues to fix but hopefully it's not too far out. Appreciate you testing it out!

And nice work on the investigation and digging! I do think this is the best approach, to normalize the 'filetype' by pulling the portion before the period. The fix looks good to me.

We actually had a similiar issue in #882 - there's a very similar line of code here:

// Fix for #882 - handle cases like `javascript.jsx` where there is

One thought I have is that, if we see more places that need this 'normalized' language from the Vim filetype, it may be worth pushing it up the stack to remove duplication, like somewhere in here: https://github.com/onivim/oni/blob/master/browser/src/neovim/NeovimInstance.ts

But I think this fix is great for now - if we see another instance of needing this 'normalized' language, we can consider doing the refactoring to push this logic into a central place at that time. I'll go ahead and bring this in - thanks for catching this and putting the PR together!

@bryphe bryphe merged commit e51fbf2 into onivim:master Dec 1, 2017
@akinsho akinsho deleted the fix-tsx-jsx-grammar branch December 3, 2017 23:23
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