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

(svelte2tsx) Allow documenting components with a tagged HTML comment #285

Merged

Conversation

fnune
Copy link
Contributor

@fnune fnune commented Jul 5, 2020

Goal

Solve #280

For example:

<!--
@component
Here's some documentation for this component. It will show up
on hover for TypeScript projects using a LSP-compatible editor
such as VSCode or Vim/Neovim with coc.nvim.

- You can use markdown here.
- You can use code blocks here.
-->

<main>
  <h1>
    Hello world
  </h1>
</main>

Single-line comments should work too. The contents of the HTML comment can then be picked up by the language service provider.

Progress

  • Use named default exports instead of anonymous, using the name of the file for the exported class, in such a way that autocompletion doesn't end up broken.
    • Maybe update the test suite to expect the more realistic case where svelte2tsx is called with a filename.
    • Transform the filename into a PascalCase name for the default-exported class.
  • Implement the documentation feature with an HTML comment.
    • Parse an HTML comment that looks like <!-- @component Documentation string -->.
    • Take the string from the tag and add it to a JSDoc on the default (now named) export.
    • Document this feature somewhere.

About the test suite

With this, the test suite is now mostly unrealistic, since all the expected strings use anonymous exports. In most (if not all?) cases, svelte2tsx would be called with a filename. Maybe I should update all the tests.

To be discussed

Edit: discussion solved, we go for @component.

Is this HTML comment approach the best? And if so, what should the tag look like? Here are some ideas:

  • @documentation
  • @doc
  • @component: I like this one better because @doc or @documentation sounds a bit vague. @component specifies what the documentation is about, and it's obvious that it's documentation.

Maybe we could support multiple, too.

@jasonlyu123
Copy link
Member

Wonder if we could allow jsdoc in this documentation block. Something like @fires, @see, @prop would be nice. However, some of it would have a different meaning.

something like this

<!--
  @component PaginationToolbar
  @props list list for pagination
  @see Pagination.md
-->

@fnune
Copy link
Contributor Author

fnune commented Jul 6, 2020

@jasonlyu123 Yup, TSDoc is what you're supposed to write in it. It will be picked up by editors and rendered correctly. Unless I'm missimg something.

The additional @component tag would be just foe svelte2tsx to be able to pick it up.

@jasonlyu123
Copy link
Member

Haven't used to refer it with the new tsdoc standard ms is pushing lol

@dummdidumm
Copy link
Member

  • About @component - good idea, I like that.
  • About the test suite issue - I'm not too worried about that. It's tested indirectly in such a way that now every test file will have export default class Input.

@fnune
Copy link
Contributor Author

fnune commented Jul 6, 2020

Haven't used to refer it with the new tsdoc standard ms is pushing lol

@jasonlyu123 I see, sorry then, I misunderstood your initial post.

The implementation I'm going to make just passes along the string to the LSP. Beyond that, I don't know if any additional work needs to be done to support what you suggested.

@fnune
Copy link
Contributor Author

fnune commented Jul 6, 2020

* About `@component` - good idea, I like that.

* About the test suite issue - I'm not too worried about that. It's tested indirectly in such a way that now every test file will have `export default class Input`.

@dummdidumm Two points arise from that statement about the test suite:

  • We would need to change the svelte2tsx/test.js file to support that. I'll do that as part of the first commit.
  • You wrote export default class Input. I was thinking of respecting the casing the user created in their file, in this case it would be input. Do you think we should capitalize it?

@dummdidumm
Copy link
Member

dummdidumm commented Jul 6, 2020

  • We would need to change the svelte2tsx/test.js file to support that. I'll do that as part of the first commit.

Sounds good!

  • You wrote export default class Input. I was thinking of respecting the casing the user created in their file, in this case it would be input. Do you think we should capitalize it?

I think so, yes, the first letter should be capitalized. To me this seems like a standard in the JS/TS world. Classes are always first letter uppercase. Same for components in svelte, at least from what I have seen. Do you know of cases where this is not the case? I think it's more likely that people use first-letter-lower file names than first-letter-lower component imports, that's the rationale behind it.

@fnune
Copy link
Contributor Author

fnune commented Jul 6, 2020

Yup, I've also always written component names capitalized. The problem is the user could be using snake case or something else. Maybe I could bring in some utility to transform any case to PascalCase.

@dummdidumm
Copy link
Member

Yes, good point, such transformations should be done.

@PatrickG
Copy link
Member

PatrickG commented Jul 6, 2020

AFAIK components needs to have an uppercase first letter to be rendered anyway (except svelte:component).

@fnune fnune force-pushed the feature-enable-docstrings-for-components branch 2 times, most recently from aed5019 to ecef276 Compare July 6, 2020 18:16
@fnune fnune force-pushed the feature-enable-docstrings-for-components branch 2 times, most recently from 39e52a5 to e6a983c Compare July 6, 2020 19:06
@fnune fnune marked this pull request as ready for review July 6, 2020 19:08
@fnune
Copy link
Contributor Author

fnune commented Jul 6, 2020

Alright! This is ready for review. I hope you don't find it too controversial :)

I've tested it locally and it's working just fine. Maybe you can find some edge case?

If you approve it, please point me to where I should document this.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Docs go into https://github.com/sveltejs/language-tools/blob/master/docs/README.md , above FAQ / Troubleshooting as its own section.

@fnune fnune force-pushed the feature-enable-docstrings-for-components branch from e6a983c to 3890afb Compare July 7, 2020 08:02
@fnune fnune force-pushed the feature-enable-docstrings-for-components branch from 0792dc0 to 4259fcf Compare July 7, 2020 08:10
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
@fnune fnune force-pushed the feature-enable-docstrings-for-components branch from 4259fcf to 3b64ffd Compare July 7, 2020 08:56
@dummdidumm dummdidumm merged commit da98889 into sveltejs:master Jul 7, 2020
@dummdidumm
Copy link
Member

🎉

dummdidumm pushed a commit to dummdidumm/language-tools that referenced this pull request Jul 8, 2020
Thanks to the work in sveltejs#285, `svelte2tsx` now creates files with named default exports, which makes some code in the `CompletionProvider` obsolete.
dummdidumm added a commit that referenced this pull request Jul 8, 2020
Thanks to the work in #285, `svelte2tsx` now creates files with named default exports, which makes some code in the `CompletionProvider` obsolete.
@jasonlyu123
Copy link
Member

Work like a champ with #301 lol
圖片

@fnune
Copy link
Contributor Author

fnune commented Jul 23, 2020

Awesome 😄

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.

4 participants