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

jsx #133

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

jsx #133

wants to merge 8 commits into from

Conversation

numToStr
Copy link
Owner

@numToStr numToStr commented Mar 25, 2022

tl;dr - I am just playing with the idea of making jsx first-class, probably via pre_hook but if the implementation gets more complicated than I hope it to be then I'll probably make this a separate plugin.


Usage

require('Comment').setup({
    pre_hook = function(ctx)
        return require('Comment.jsx').calculate(ctx)
    end,
})

Why?

  • It's fun to play with treesitter.
  • I personally don't like the verbose integration with context-commentstring.
  • jsx is the only thing that is missing to make this plugin a masterpiece.
  • And lastly, one less plugin to install(?)

What's missing?

In its current state, it works but does have some issues regarding accuracy

1. Can't comment/uncomment multiple attributes at once 1 Fixed using workaround

Details
  • Actual
<p
  {/* he="llo" */}
  {/* wor="ld" */}
  para="graph"
>
  this is a p tag
</p>
  • Expected
<p
  // he="llo"
  // wor="ld"
  para="graph"
>
  this is a p tag
</p>

(Also when uncommenting, expecting // to be used instead of {/* */})

2. Can't uncomment the following at once 1 Fixed using workaround

Details
<div>
  {/* <section> */}
  {/*   <p>hello</p> */}
  </section>
</div>

(This is not a priority, as the syntax is already broken)

  • Actual
<div>
  // {/* <section> */}
  // {/*   <p>hello</p> */}
  </section>
</div>
  • Expected
<div>
  <section>
    <p>hello</p>
  </section>
</div>

FAQ

Why pre_hook is needed?

Because everyone is not using jsx so if you need it then that's da way.

Is this going to be shipped with the plugin?

As I said it depends on the implementation. If it's small, simple, and covers most cases then I'll merge it.

Any other issues?

I don't use jsx that much, so if you find something feel free to comment. And to be clear I am not aiming for 100% accuracy but ~90%. So, there is a chance that some issue won't get fixed.

Can I use it? Is this stable?

Yes, you can definitely use it. And as you know JS is JS so this will always remain experimental.

Footnotes

  1. Both of them are a non-issue if you comment/uncomment them individually 2

@numToStr
Copy link
Owner Author

numToStr commented Mar 25, 2022

For testing

sample.tsx
const Yoo = () => {
  return (
    <div>
      <section>
        <p>hello</p>
        <p
          he="llo"
          wor="ld"
          attr={{
            ...window,
            hello: () => {
              return (
                <section>
                  <p>IDK</p>
                </section>
              );
            },
          }}
        >
          hello
        </p>
        <p>{true ? "true" : "false"}</p>
        <p>{true && "true"}</p>
        <p>
          {true && (
            <section>
              <p>This is awesome</p>
            </section>
          )}
        </p>
        <div id="div">
          {getUser({
            name: "numToStr",
            job: "making plugins",
          })}
        </div>
      </section>
      <div className="flex items-center justify-center image-uploader">
        <span className="animate-ping h-4 w-4 rounded-full bg-sky-400 opacity-75"></span>
      </div>
    </div>
  );
};

@numToStr numToStr force-pushed the jsx branch 2 times, most recently from 0fe68bd to dd60ffa Compare April 6, 2022 08:20
@numToStr

This comment was marked as resolved.

lua/Comment/jsx.lua Outdated Show resolved Hide resolved
@kuntau
Copy link

kuntau commented Apr 9, 2022

Excellent! Waiting for this 🥂

@numToStr
Copy link
Owner Author

numToStr commented Apr 9, 2022

@kuntau It would be helpful if you can test this out and report any cases where comments are not correct apart from the issues that are already listed :)

@numToStr
Copy link
Owner Author

Previously mentioned issues are now fixed (workaround) so this is now usable (I am using it) but there might be some cases where different commentstring might get used. I am leaving this PR for further testing.

To be clear I am not aiming for 100% accuracy but ~90%. So, there is a chance that some issue won't get fixed.

@numToStr numToStr force-pushed the jsx branch 3 times, most recently from 28cd517 to 7a284b9 Compare May 30, 2022 05:27
@cusxio
Copy link

cusxio commented May 31, 2022

I was trying this out, it seems that this doesn't trigger for .tsx files? Works great for .js files though, though it seems that it comments individual lines rather than a block. Is that the expected behaviour?

CleanShot.2022-05-31.at.08.42.28.mp4

@numToStr
Copy link
Owner Author

I was trying this out, it seems that this doesn't trigger for .tsx files?

Works for me though. Do you have tsx parser installed?

though it seems that it comments individual lines rather than a block. Is that the expected behaviour?
https://user-images.githubusercontent.com/6487613/171071876-37a03f90-9361-4fb4-bcaa-b865c21fcc77.mp4

I think you are pressing gc in this clip. Try with gb.

@cusxio
Copy link

cusxio commented May 31, 2022

I wasn't aware that there's a tsx parser, I only had the typescript one installed.

Everything seems to work as expected, including gb and gc behaviour! Thank you!

@ecosse3
Copy link

ecosse3 commented May 31, 2022

Seems to work fine when we just have JSX elements around, but breaks when I have normal HTML element like <option>.

Comment-nvim-tsx-bug

@ShiChenCong
Copy link

seems has problem with Jsx Component with props(use gc)
image
image

@numToStr
Copy link
Owner Author

numToStr commented May 31, 2022

@ecosse3 @ShiChenCong Nice catch! I'll try to fix those issue.

@numToStr
Copy link
Owner Author

numToStr commented Jun 2, 2022

@ShiChenCong Is that a self closing element in the image? Can you reply with the element that you are facing issue with, if that's ok.

@smithbm2316
Copy link

@numToStr I've been using this PR for the last few days and have not noticed any weird behavior whatsoever. Seems to be working quite great in a React environment for me so far! Thanks for the hard work on this 😄

@amirhhashemi
Copy link

amirhhashemi commented Jun 4, 2022

I can confirm the bugs that @ecosse3 has reported. It sometimes can't comment regular HTML tags correctly. for some reason if I remove the className it works!

2022-06-04.11-38-27.mp4

Also:

2022-06-04.11-43-01.mp4

The jsx:

<NodeViewWrapper className="flex items-center justify-center image-uploader">
      {loading && (
          <span className="animate-ping h-4 w-4 rounded-full bg-sky-400 opacity-75"></span>
      )}
</NodeViewWrapper>

@numToStr
Copy link
Owner Author

numToStr commented Jun 4, 2022

@ahhshm I was able to reproduce the same.

My observation is that any element that has attributes on the same line alongside the opening element is using wrong comment string. It doesn't matter whether it's a custom or native elements.

@ShiChenCong
Copy link

ShiChenCong commented Jun 5, 2022

@numToStr Sorry for reply so late, yes, It is self closing element

@numToStr
Copy link
Owner Author

numToStr commented Jun 5, 2022

@ShiChenCong No problem. Now it should work with self closing element.

@numToStr
Copy link
Owner Author

@ahhshm @ecosse3 I pushed a fix that should resolve the issue that you were facing before.

@amirhhashemi
Copy link

amirhhashemi commented Jun 18, 2022

@numToStr Sorry for the late response.

It seems like updates have solved all of the previous problems. I just found a small bug, it has a little problem with uncommenting opening fragment tags:

2022-06-18.12-08-23.mp4

with ts_context_commentstring:

2022-06-18.12-16-33.mp4

The jsx (for testing):

    <NodeViewWrapper as="figure" dir="ltr" data-type="custom-image">
      {node.attrs.src && (
        <>
          <img
            src={node.attrs.src}
            alt="Image"
            className={`${node.attrs.src.startsWith("data:") && "blur-sm"}`}
          />
          <bdi>
            <NodeViewContent as="figcaption" className="text-center" />
          </bdi>
        </>
      )}
    </NodeViewWrapper>

@numToStr
Copy link
Owner Author

numToStr commented Jun 18, 2022

@ahhshm hmmm...this also seems to be broken in context-commentstring. I'll try to fix this.

@amirhhashemi
Copy link

amirhhashemi commented Nov 23, 2022

Any update on this? I've been using this branch since it was created and it's working fine for me. I haven't encountered any specific bug in a while and although it's a WIP, it seems pretty stable. Anything that I can help you with to merge it into the main beach sooner?
No pressure btw. I just want to know when I can remove branch="jsx" in my packer config:)

@numToStr
Copy link
Owner Author

@ahhshm I am really happy to hear that this is working without any major hiccups. As I said earlier, my main reason for holding this out is because

Both would've helped in making the implementation robust and much nicer but it seems both PRs are kinda blocked, sadly.

@rick-yao

This comment was marked as off-topic.

@CoderLadFahim
Copy link

I'm getting this message every time I attempt to comment in jsx.

image

I believe I've configured the pre_hook properly:
image

@CoderLadFahim
Copy link

Update:

Updating the pre_hook now shows the nil error everywhere, not just in JSX files.
I've tried to just call require on 'Comment.jsx' and it throws a not found error.

Please help.

@lucasmarinb
Copy link

lucasmarinb commented Jul 15, 2023

Update:

Updating the pre_hook now shows the nil error everywhere, not just in JSX files. I've tried to just call require on 'Comment.jsx' and it throws a not found error.

Please help.

This is also happening to me.

@pierregoutheraud
Copy link

Any news about this? I am getting [Comment.nvim] nill message when using this branch.

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

Successfully merging this pull request may close these issues.