corneliusio / svelte-sublime

💡Sublime Text syntax highlighting for Svelte components
MIT License
58 stars 6 forks source link

Make sure attributes with inline JavaScript are correctly closed #18

Closed ccampbell closed 2 years ago

ccampbell commented 2 years ago

Fixes corneliusio/svelte-sublime#16 Fixes corneliusio/svelte-sublime#17

There may be a better way to fix this, but this seems to work for me. It would be nice to have some syntax tests in this repo to be able to verify that things work in more unusual use cases.

corneliusio commented 2 years ago

@ccampbell Tested this out locally on my machine and this did not end up fixing the issue for me—will be working on it though. Also, note that Svelte.sublime-syntax.yaml-macros should be the source file where changes are made, which is then compiled to Svelte.sublime-syntax with the YAML Macros package/plugin, just for future reference.

ccampbell commented 2 years ago

Hmmm. For me it definitely works. Did you remove all active svelte syntaxes before installing it? I am on Sublime build 4120 on the dev channel (should be the same as the 4121 stable build), but the highlighting broke for me earlier than that (dev channel gets updated more often).

Before

After

As for the other stuff, sorry about that, I will fix that up so it uses the macros file.

ccampbell commented 2 years ago

An easy way to test is to use safe mode.

Mac instructions:

  1. Quit sublime
  2. Hold option key when starting sublime
  3. $ cd ~/Library/Application\ Support/Sublime\ Text\ \(Safe\ Mode\)/Packages/User
  4. $ git clone git@github.com:ccampbell/svelte-sublime.git
  5. Check syntax

I updated it to use the macros file and YAMLMacros correctly in the this branch: https://github.com/ccampbell/svelte-sublime/tree/macros

Oddly enough, a lot of things seem to change in the .sublime-syntax file when doing it this way including removing the name attribute which seems odd to me.

corneliusio commented 2 years ago

@ccampbell It's very possible the HTML syntax definition that this extends also had updates made to it which would carry over when the source file is built. I should clarify that your solution did partially fix the problem, however there was something odd going on with the syntax definitions in the embed JS scope.

It's very possible I'll only need to build on what you changed, honestly I haven't had to do any extensive work with Sublime's syntax definitions for a while so a little rusty and just getting back into the groove. I'll also double check that I didn't have any other conflicts going on that may have been the root of any issues I'm seeing on my end.

Appreciate your time, work, and patients on this. Will be trying to get it worked out this weekend while I'm out of town as much as possible.

corneliusio commented 2 years ago

I also need to make sure this is backward compatible with Sublime Text 3 and if it's not, create separate releases for them.

deathaxe commented 2 years ago

If you built a new file using ST4 the resulting syntax won't be backward compatible as core HTML.sublime-syntax is of version 2 which ships some breaking syntax engine features/fixes.

An ST4 only release could simplify syntax definition by extending HTML.sublime-syntax

To learn how it works you may have a look at https://github.com/vuejs/vue-syntax-highlight/tree/st4. They use nearly the same approach as this package.

corneliusio commented 2 years ago

@deathaxe Thanks for the info, I had misspoken in my previous comment—it looks like I included a copy of the HTML syntax definitions in the package to avoid that potential issue. And I just discovered the new extending features in ST4 today while working through the broken Javascript syntax include. I will def be referencing vue-syntax-highlight for that ST4 specific release, they were the original inspiration for much of the solutions in this package. I'll likely need to rewrite most of the definitions from scratch due to this new approach, but for the time being this older, somewhat janky solution should work.

@ccampbell Thanks again for your time helping get this bug resolved. I'm closing out this PR in favor of @deathaxe's fix posted in the issue comments.