Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toggle comments broken on v10.0.0-beta2 #390

Closed
brupelo opened this issue Aug 24, 2020 · 17 comments
Closed

Toggle comments broken on v10.0.0-beta2 #390

brupelo opened this issue Aug 24, 2020 · 17 comments
Labels

Comments

@brupelo
Copy link

brupelo commented Aug 24, 2020

Version: v10.0.0-beta2

Consider this little snippet:

export default function Foo(props) {
  return (
    <div>
      <Foo>
      </Foo>
    </div>
  );
}

And now try to comment/uncomment an inner block of code, ie:

showcase

You can see 2 examples of comment/uncomment not working:

  1. If i select a block of code and comment/uncomment without selecting anything else will fail
  2. If i select a block of code, comment and then I reselect the entire commented block it will also fail
@brupelo
Copy link
Author

brupelo commented Aug 24, 2020

I don't know much about syntax highlighting in SublimeText but for those maintaining this project there is a really good package called Scope Hunter that could be useful

@Thom1729
Copy link
Contributor

Thom1729 commented Apr 5, 2021

Is this a regression compared to previous versions? This is definitely something I'd like to find a way to improve either way.

@Thom1729
Copy link
Contributor

I've submitted a PR upstream to improve commenting behavior in JSX (sublimehq/Packages#2787).

@skizzo
Copy link

skizzo commented Apr 13, 2021

I have the exact same problem, and am looking for a solution. On my other development environment, Sublime Text 3 still uses v8.6.3 of this package, and it's working there.

So my question is: Does anybody know how to force Sublime Text to use a specific version of a package?

@Thom1729
Copy link
Contributor

I don't think Package Control supports pinning a package version. If this is breaking your workflow, you could uninstall via Package Control and check out the previous tag via git.

Either way, if this is a regression from the previous version, I intend to fix it. I know you said you had exactly the same problem as @brupelo, but just to be sure, can you post a code example?

I tried @brupelo's example in both the current release and the previous release, and neither of them does the right thing.

@skizzo
Copy link

skizzo commented Apr 13, 2021

Okay, thanks for the info about the package version. Here's an example and what happens when I try to comment/uncomment using the keyboard shortcut:

SublimeText3ReactSyntaxHighlightingComment.mp4

@Thom1729 Thom1729 added the bug label Apr 13, 2021
@Thom1729
Copy link
Contributor

Yeah, this is definitely a bug. This case was also broken on the old version, so it's not a regression, but it still deserves a fix.

I've already submitted a fix for the ST4 JSX package. That particular fix will end up in babel-sublime when:

  1. The PR is merged (and I've rebased JS Custom).
  2. I release a ST4 build for babel-sublime.

Depending on how things look over the next few days, it may make sense to backport something to v10.

@Thom1729
Copy link
Contributor

@skizzo I've released version 10.0.2 with a fix for that specific case. It's monkeypatched onto the compiled syntax definition, which is not great, but when v11 comes around everything should be in accord.

The fix does not address the issue @brupelo reports; that part of the upstream fix can't easily be backported for ST3.

@skizzo
Copy link

skizzo commented Apr 15, 2021

Thank you a lot @Thom1729 I can indeed confirm that the case I posted above is solved now with version 10.0.2!

I have only one more issue with the 10.0.2 vs. 8.6.3 topic:

SublimeText3ReactSyntaxHighlightingComment2.mp4

Is there any chance you could fix this as well? Thanks!

@Thom1729
Copy link
Contributor

This should be fixed in v10.0.3.

@skizzo
Copy link

skizzo commented Apr 16, 2021

Thank you for the update, but it's behaving the exact same way as before, tested on 2 machines.

@Thom1729
Copy link
Contributor

That would be because I forgot to push the commits before releasing. Try 10.0.4.

@skizzo
Copy link

skizzo commented Apr 16, 2021

Thanks, but I just updated to 10.0.4, restarted Sublime, but it's still the same..

@Thom1729
Copy link
Contributor

I typed out the example from your image and ran some tests. The behavior varies between the new and old versions of Babel, but also depending on whether you have the ST4 JavaScript package, the ST3 JavaScript package, or whether the JavaScript package is disabled. The only case in which it works correctly is with the old babel-sublime and the ST3 JavaScript package enabled.

The root cause of the issue is sublimehq/sublime_text#2152. When Sublime tests the scope source.js meta.jsx source.js.embedded, the selector meta.jsx will outscore source.js, which is arguably wrong. This is inevitably going to cause problems with alternating nested scopes like the one in your example. (I tested with simpler code that did not include alternating nesting, and v10.0.4 should improve behavior in similar cases.)

It may be possible to hack the comment rules to produce better results in your example. But I'm thinking that the best solution might be to write an alternate commenting command that handles these cases in a more robust manner. (See, for example, the jsx_close_tag command from JS Custom.) Unfortunately, that's probably not a quick fix. I'll play around with this today and see how it looks.

@Thom1729
Copy link
Contributor

Thom1729 commented May 4, 2021

The v11 beta is out, which should slightly improve JSX commenting.

@skizzo
Copy link

skizzo commented May 5, 2021

Thanks a lot for your work mate, it's getting better and better!

@Thom1729
Copy link
Contributor

v11 is out. Commenting should work correctly, except for nested situations due to sublimehq/sublime_text#2152. Any other commenting bugs should be reported in a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants