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

@slot and @event description bugs #102

Open
endigo9740 opened this issue Oct 24, 2022 · 11 comments
Open

@slot and @event description bugs #102

endigo9740 opened this issue Oct 24, 2022 · 11 comments

Comments

@endigo9740
Copy link

Hey, my name is Chris and I'm the project lead and core contributor to Skeleton:
https://skeleton.dev/

We're in the process of integrating Sveld to automatically document our various components in our library. Unfortunately we're running into a couple issues with the new @slot and @event description fields introduced here:

Here's how we're defining our JSDoc comments for a component. This includes 3x Slots and 1x forwarded Event:

Screen Shot 2022-10-24 at 5 34 24 PM

Issue 1 - First Slot Always Dropped

NOTE we do not use the default slot on this particular component.

Per our JSDoc definitions above you'll see we're adding an empty @slot definition, which should come out to a total of 4x slots. But we've noted that regardless of how we structure or order the comments, the first @slot definition is always pruned. The only work around we've discovered is to insert an empty slot to be "sacrificed".

Screen Shot 2022-10-24 at 5 35 00 PM

Issue 2 - @event changes types

We've applied a single on:click event to our template. This should be forwarded up. Here's what that looks like if we do not specify a JSDocs comment, just allow Sveld to auto-document it:

Screen Shot 2022-10-24 at 5 38 57 PM

The above is correct, though missing the description obviously since one isn't specified.

However, when we do specify a description, we see two issues:

  • The type information incorrectly changes from forwarded -> dispatched
  • We lose the element value - though I believe this is due to the fact it's being treated as "dispatched"

Screen Shot 2022-10-24 at 5 38 27 PM

FYI we're still testing, so we might update this post or create another if we discover other issues. However, these are the two most pressing we've encountered.

@endigo9740
Copy link
Author

endigo9740 commented Oct 25, 2022

Note I've just noticed Issue 2 may be intended behavior per your README instructions:

@event
Use the @event tag to type dispatched events.

We would love to be able to add descriptions to forwarded events, but I'd understand if it's not feasible right now.

So that just leave issue 1

@metonym
Copy link
Collaborator

metonym commented Oct 25, 2022

Hi Chris, thank you for reporting this, along with the detailed screenshots.

Come to think of it, I don't think there's any reason why @event should only be limited to dispatched events. sveld should be able to determine if an event is forwarded, and use the correct type.

The same goes for default slots not allowing a description.

@metonym
Copy link
Collaborator

metonym commented Oct 25, 2022

@endigo9740 For the first issue, could you try adding an explicit type annotation for the default slot?

- @slot
+ @slot {{}}

I'm testing this with the Sveld GUI (https://sveld.onrender.com/), and the @slot without a type annotation throws an error. With the explicit {{}}, I get the expected TS/JSON output.

Screen Shot 2022-10-24 at 5 19 44 PM

@endigo9740
Copy link
Author

endigo9740 commented Oct 25, 2022

@metonym That would be awesome if we could handle both scenarios.

For our project we use the default slot for different reasons, so being able to specify the intention would be great. In a lot of cases it's a set of child elements, like a set of Tabs for a Tab Group.

I think most of our events will be forwarded default Svelte events (on:click, etc), so we can try and communicate that, but having it be explicit with the description would be great.

I'm away from my workstation right now so I'll ask another contributor to test your suggestion above and we'll report back shortly.

@endigo9740
Copy link
Author

endigo9740 commented Oct 25, 2022

@metonym FYI I noted you're using the multi-line comment format, whereas I was using single line, so I've corrected that on my end. That was the source of some of my troubles, so that's my bad!

As for your suggestions above - comparing notes with @niktek, our contributor, we're seeing some strange results. In the "onRender" playground we can replicate the results you shown above 1:1, but locally in our project results do not match.

In the playground:

Screen Shot 2022-10-24 at 7 53 18 PM

In our project:

Screen Shot 2022-10-24 at 7 54 03 PM

Screen Shot 2022-10-24 at 7 53 48 PM

It's like it's killing all the descriptions for some reason.

I'm not sure if it's relevant, but FYI we're utilizing Sveld via vite-plugin-sveld v1.1.0. Perhaps this is influencing things? We originally tried setting up Sveld as a Rollup plugin in our SveldKit project, but weren't able to get this working, so we opted for the plugin. If you have any guidance that could help we could try that again. Perhaps eliminate one point of failure here?

@metonym
Copy link
Collaborator

metonym commented Oct 25, 2022

Given that vite-plugin-sveld now specifies sveld as a peer dependency, could you double-check the installed sveld version?

@endigo9740
Copy link
Author

It appears to be 0.18.0. I wouldn't expect @slot or @event descriptions to work otherwise, correct?

Screen Shot 2022-10-25 at 1 10 13 PM

@endigo9740
Copy link
Author

endigo9740 commented Oct 25, 2022

FYI I've been testing a few combination of comment formats. Here's what does or doesn't work:

Multiple single-line comments with an empty slot as the first definition - this works:

Screen Shot 2022-10-25 at 1 11 44 PM

Multiple single-line comments but replacing the empty slot with just any regular // comment - this works:

Screen Shot 2022-10-25 at 1 07 17 PM

Multi-line comment format - this does not work:

3

I've tried various versions of the multi-line comment, but none of these work.

@metonym
Copy link
Collaborator

metonym commented Oct 25, 2022

I am surprised that the last example does not work.

Given the input:

<script>
   /**
     * @slot {{}} content - This is the CONTENT slot description.
     * @slot {{}} lead - This is the LEAD slot description.
     * @slot {{}} summary - This is the SUMMARY slot description.
     */

   /**
     * @event {{}} click - This is the CLICK event description.
     */
</script>

I see this in the JSON output:

Screen Shot 2022-10-25 at 11 46 07 AM

I don't think it's a problem with vite-plugin-sveld since all it does is use the ComponentParser API. At the point, I would try to delete the Vite cache (i.e., node_modules/.vite to eliminate that as a possibility.

@endigo9740
Copy link
Author

endigo9740 commented Oct 25, 2022

I deleted the VIte cache per your instruction and interestingly enough your example above doesn't work, but when I keep the leading // comment it does. This is the ONLY way I've managed to get the multi-line comment structure working for slot definitions. So yay progress! :)

Screen Shot 2022-10-25 at 1 57 38 PM

// Slots:
/**
 * @slot {{}} content - Allows for an optional leading element, such as an icon.
 * @slot {{}} lead - Provide the summary details of each item.
 * @slot {{}} summary - Provide the content details of each item.
 */

Removing that leading comment makes all the descriptions disappear.

@endigo9740
Copy link
Author

endigo9740 commented Oct 25, 2022

FYI If you want to replicate what we're seeing, you can. Skeleton is open source, so you can pull down and run a version locally if you wish. This is a standard SvelteKit project.
https://github.com/Brain-Bones/skeleton

EDIT
We've merged our update into our primary dev brach, so updating the instructions here. Everything else should be accurate below.

The component I'm testing this on is located in src > lib > components > Accordion > AccordionItem.svelte, with the @slot definitions right at the top of the file. And you can preview it here in the local server:

http://localhost:5173/components/accordions

Just make sure to tap the Slots tab. Also we've noted the HMR doesn't work with Sveld, so a manual page refresh is needed between code changes for anything Sveld-related.

I'm happy to walk you through how we're passing the data around if need be. Otherwise here's a direct link to the file on GitHub if you want to just do a quick scan and see if anything jumps out at you:
https://github.com/Brain-Bones/skeleton/blob/dev/src/lib/components/Accordion/AccordionItem.svelte

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

No branches or pull requests

2 participants