maciekgrzybek / svelte-inview

A Svelte action that monitors an element enters or leaves the viewport.🔥
MIT License
749 stars 23 forks source link

Export type definitions #5

Closed jpoep closed 2 years ago

jpoep commented 3 years ago

Hey,

first of all - pretty cool Svelte component, thanks for sharing it! :)

One issue I have, though, is that the package doesn't seem to expose the type definitions you wrote, so my compiler is giving me warnings on import and even a false error when using any of the on:-actions:

(JSX attribute) onenter: () => void Type '{ onenter: () => void; }' is not assignable to type 'HTMLProps'. Property 'onenter' does not exist on type 'HTMLProps'.ts(2322)

It'd be neat if you could export your type definitions so it's compatible with TypeScript projects out of the box.

maciekgrzybek commented 3 years ago

Hey @qelix Thanks for comment 🙂 Yeah that's a fair point, I will sort it out today 😉

maciekgrzybek commented 3 years ago

Unfortunately, this is far more complicated than I thought. I'll try to sort it out ASAP but if you have an idea how to fix it, feel free to create a PR :)

Greenheart commented 3 years ago

Great work with this module! 🎉

Maybe this could help to generate .d.ts files? https://github.com/rollup/plugins/issues/61#issuecomment-597090769

maciekgrzybek commented 3 years ago

Yeah, that doesn't help as well, unfortunately... EDIT: Looks like this plugin rollup-plugin-typescript2 is generating something, fingers crossed 😄

jpoep commented 3 years ago

Thanks for the fast reply!

I think I also got it working using rollup-plugin-typescript2. If you didn't succeed on your own yet, I can submit the PR.

danawoodman commented 3 years ago

I'm getting the following TS errors:

Type '{ onenter: (event: any) => void; width: string; height: string; viewBox: string; fill: string; xmlns: string; }' is not assignable to type 'SVGProps<SVGSVGElement>'.
  Property 'onenter' does not exist on type 'SVGProps<SVGSVGElement>'.ts(2322)
(JSX attribute) onenter: (event: any) => void

I assume the issue here is you're extending the event handling for a generic HTML element which TS isn't aware of.

I'm wondering if the API would be more intuitive if you instead passed these handlers in as part of the options object?:

<script>
  import { inview } from "svelte-inview";

  let isInView;
  const options = {
    onEnter(event) {
      isInView = true
    },
    onLeave(event) {
      isInView = false
    },
    onChange(event) {
      console.log('changed')
    },
  };
</script>

<div use:inview={options}>...</div>

I believe this approach would not necessitate any TS changes and in theory "just work". Plus, this feels a bit more inline with how Svelte actions are normally constructed.

Not sure if there is a limitation/issue with this approach, but it would at least solve the above issue I think.

maciekgrzybek commented 3 years ago

Hey, thanks for that. That was my first idea, but I decided to go with the example from the Svelte docs, where they are using the on:event syntax. I know how to extend the types (taken from language-tools docs):

export declare namespace svelte.JSX {
  interface HTMLProps<T> {
    oninviewchange?: (details: Details) => void;
  }
}

This solution works when I put those types in the project that's using svelte-inview, but I can't make it work with the package itself. What I mean is, if I export the types.d.ts file in the package, the project that's using it is not picking up those types and I don't know how to make it work. If you have any idea how to fix it, I'm listening :D I'm happy to pair on it or collaborate in some different way :) Hope that all makes sense?

danawoodman commented 3 years ago

Ah I see, that makes sense.

Have you asked on the Svelte Discord? I wonder if someone there might have an idea?

If I have some time early this week I will also look to see if I can find a solution. My first guess is maybe it is a TS config issue and/or how the declaration is overridden, but I'm not too sure.

The one tricky bit would be how would you extend that type but only for elements that have use:inview as those event types would be invalid otherwise. I wonder if there is a way to have language tools only extend the type when the action is used? Perhaps someone in the language tools Discord would have an idea?

maciekgrzybek commented 3 years ago

Hey, @danawoodman totally forgot to reply 🤦 Yep I've asked on the discord, no luck :) I think I'll use your first proposal and move those methods to the option. It makes more sense really :)

maciekgrzybek commented 2 years ago

This is sorted now 👌