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

Add TypeScript defintion #61

Closed
nmussy opened this issue Jul 29, 2019 · 18 comments · Fixed by #77
Closed

Add TypeScript defintion #61

nmussy opened this issue Jul 29, 2019 · 18 comments · Fixed by #77

Comments

@nmussy
Copy link

nmussy commented Jul 29, 2019

Hey,

It'd be nice to add TypeScript definitions to the project. I can make a PR if needed.

EDIT: Looking at how compact the src/ is, it might even be worth switching the project to TypeScript, if you feel up to it

@Wildhoney
Copy link
Owner

I'd rather keep it as standard JS.

Would be awesome to have a definition file though – thanks @nmussy!

@nmussy
Copy link
Author

nmussy commented Jul 29, 2019

No problem, I'll probably make a PR tonight. It should be pretty easy to maintain, but you can @ me in the future if you need me to update it.

@Wildhoney
Copy link
Owner

Thank you @nmussy 👍

@schibrikov
Copy link

Also wanted to use the package in TS, but ran into definitions absence.
I'm still not sure I need to use it, but if I decide, I can create definitions myself.

Any news on this? @nmussy

@nmussy
Copy link
Author

nmussy commented Aug 2, 2019 via email

@Wildhoney
Copy link
Owner

Any luck folks?

@nmussy
Copy link
Author

nmussy commented Aug 12, 2019

Still haven't looked into it more, sorry. I'll open a PR with what I have so far tonight

@MalekBouba
Copy link

Same issue.. can we help?

@nmussy
Copy link
Author

nmussy commented Aug 29, 2019 via email

@yurist38
Copy link
Contributor

I'm about to start using it and missing TS definitions too... If you guys have any draft of definitions and have no time to complete, please commit the stuff and let us know. Some of us can contribute to making it done, I'm pretty sure.

@yurist38
Copy link
Contributor

yurist38 commented Oct 7, 2019

I made my try in the end and that's what I ended up with for now:

declare module 'react-shadow' {
  import { ReactNode, ComponentType } from 'react';

  type RenderElement = keyof HTMLElementTagNameMap;

  type Root = {
    [name in RenderElement]: ComponentType;
  }

  const root: Root;

  export default root;
}

If you have any idea what is wrong or how I can improve these typings - please feel free to share!

@nmussy
Copy link
Author

nmussy commented Oct 7, 2019 via email

@marijnbent
Copy link

Same problem here. Any luck fixing it?

@benji6
Copy link

benji6 commented Jan 9, 2020

@yurist38 your types were really helpful for me. I think this might be a slight improvement:

declare module "react-shadow" {
  import * as React from "react";

  type RenderElement = keyof HTMLElementTagNameMap;

  type Root = {
    [name in RenderElement]: React.ComponentType<
      React.HTMLAttributes<HTMLElementTagNameMap[name]>
    >;
  };

  const ReactShadowRoot: Root;

  export default ReactShadowRoot;
}

I'm just passing a type argument of React.HTMLAttributes<HTMLElementTagNameMap[name]> to React.ComponentType. It's not quite right though because if the element is a button for instance we should be using React.ButtonHTMLAttributes instead of React.HTMLAttributes, but it did solve a type error I was getting.

@Wildhoney
Copy link
Owner

Still no luck guys?

@yurist38
Copy link
Contributor

I've opened a PR based on the solution from @benji6 (thanks for that!). Let's see if it's going to be merged...

@Wildhoney
Copy link
Owner

@yurist38 just a question of how HTMLElementTagNameMap works if you're using custom elements? For example root.myCustomElement that yields a <my-custom-element> node is perfectly fine in react-shadow, whereas HTMLElementTagNameMap seems to be a map of only the standard HTML elements.

Wouldn't that cause issues? Do we need to allow for arbitrary string types, too?

@yurist38
Copy link
Contributor

@Wildhoney yes, your concern was correct. Just tested it locally, it doesn't work with a custom element. So I think you're right and we have to make it less strict... I'm opening a PR with refactoring to string.

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 a pull request may close this issue.

7 participants