Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Could not find a declaration file for module 'accessible-astro-components' #40

Closed
Hugo-Persson opened this issue Mar 3, 2023 · 10 comments
Assignees
Labels
bug Something isn't working

Comments

@Hugo-Persson
Copy link

When I use the library I get the following error, I think that the library is missing typescript type definitions.

@markteekman
Copy link
Member

Hey @Hugo-Persson! Thanks for submitting the issue 😄 I think you forgot to paste in the error? This library is currently build using vanilla JavaScript. I do not have the necessary knowledge (yet) to transform it to TypeScript.

@markteekman markteekman added the more info required Further information is requested label Mar 7, 2023
@david-abell
Copy link
Contributor

The error is likely Could not find a declaration file for module 'accessible-astro-components'. I also ran into this a few days ago. I also didn't see the repo on definitelyTyped. After muddling around with the package declared as any, I decided to just write some. See this declaration file in an accessible astro fork

This gives Prop type safety and autocomplete but doesn't check children right now for things like Breadcrumbs and Accordian. I'd have to check with the Astro crew to see if that's possible as I couldn't get it to restrict return types.

I'm not 100% sure on on all the types yet. There are a few props that have default parameters that I have left as optional that might be better off required.

I'd be happy to submit a pull request here if you'd like to add it to this repo or rework it to submit to definitelyTyped.

@markteekman
Copy link
Member

Hey @david-abell, thanks for elaborating on this issue and taking the time to create an example. I'll have to take your advice on this one, seeing as I don't have much experience with TypeScript 🤔 Would reworking it to definitelyTyped solve the issue? Or would it be better to add further TypeScript support like how you did it in your fork?

Also, thanks for suggesting to do a pull request on the matter, I appreciate all the help I can get 😊 Ever since launching v2 of the Accessible Astro Starter I've been very short on free time. Let me know and then I'll assign the issue to you 😄

@markteekman markteekman added bug Something isn't working and removed more info required Further information is requested labels Mar 15, 2023
@david-abell
Copy link
Contributor

Happy to help Mark, these are great little components to work with. While I'm no typescript master myself, I would personally recommend adding the types to the repo rather than definitelyTyped as the user experience is easier by not having to also install the extra definitelyTyped module and the repo code impact is minimal. The types would need to be updated manually either in repo or through definitelyTyped and I think its easier to not forget about them if they are kept together.

I've done a quick test with a local repo and its just a matter of having the types as an index.d.ts file. This works with no modification to to the existing components. The downside to this approach (and in using definitely typed) is that because the types are not automatically generated, the types will need to be manually updated if components or component props are updated. That being said, they aren't complicated either.

It is also possible to move component Prop type declarations into the Frontmatter of the components themselves since they are .astro files. There isn't any difference for the consumer between the two approaches as far as I'm aware but this might be less brittle since it would be using the astro component function signature instead of redeclaring it. The only maintenance then should be ensuring interface Props has the same structure as {...} = Astro.props. Just to be clear, this will need to be done no matter which method we go with since Astro and typescript aren't repo dependencies, it just puts them more obviously next to each other this way.

I'll want to check with the Astro team before pulling the trigger on either of these methods since I can't find a code example but moving props to the components would look like this:

//add interface Props to Card.astro frontmatter

interface Props {
  url?: string;
  img?: string;
  title?: string;
  footer: string;
}

const { 
  url = '#', 
  img = 'https://fakeimg.pl/640x360', 
  title = 'Default title', 
  footer = 'Your name'
} = Astro.props

// index.d.ts

type CardComponent = typeof import("./Card.astro").default;
export const Card: CardComponent;

// instead of:

// no changes to Card.astro

// index.d.ts

 interface CardProps {
   url?: string;
   img?: string;
   title?: string;
   footer: string;
 }

 export function Card(_props: CardProps): unknown;

Let me know if you have a preference.

@markteekman
Copy link
Member

Hey @david-abell, first off, thanks for your extensive research and examples on the topic! You've made a clear understanding of what is possible 😊 Secondly, thank you for your patience! I know I'm not that quick to reply, all due to being short on free time lately, so I appreciate your help.

As for the options your suggesting, adding interface Props to each component seems to be the most straightforward solution. Just recently @shawninder did a PR for the Navigation.astro component in the Accessible Astro Starter, removing some of the TypeScript errors: https://github.com/markteekman/accessible-astro-starter/pull/55/files. It might be worth considering adding these type definitions to the <script>'s code as well?

Let me know what you think!

@david-abell
Copy link
Contributor

Hey @markteekman happy to help. I'm not currently employed so I've got nothing but free time right now 😄.

I should have a pull request ready somewhat soon. I'm finishing up some basic jsDoc parameter descriptions first since otherwise the way Astro processes components seems to hide any useful descriptions.

I've had to add "prettier-plugin-astro": "^0.8.0" and a .prettierrc to keep styling consistent. I've grabbed the config from accessible astro starter

I don't think its worth updating the component scripts in these components to typescript unless you want to start learning the language . The component scripts aren't exposed or type checked by Astro so no typing errors in them will be exposed to the users. They'll just work regardless of whether the project is TS or JS. It would also add dependencies to the project since Typescript would be required.

@david-abell
Copy link
Contributor

Hey @markteekman Sorry to ping you again immediately, I was just going over diffs and realized the .prettierrc from accessible astro starter doesn't match the formatting in this project. Do you have some local settings I can add or is it manually formatted?
The pagination page is doing this and I am unable to fix it with the printWidth setting...I'd rather not turn of prettier if it can be helped.

Original

<li>
      {
        firstPage
          ? <a href={firstPage} aria-label="Go to the first page"><svg aria-hidden="true" width="32" height="32" viewBox="0 0 32 32" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M24.6667 9L18 15.6667L24.6667 22.3333" stroke="currentColor" stroke-width="2.66667" stroke-linecap="round" stroke-linejoin="round"/><path d="M14.6667 9L8 15.6667L14.6667 22.3333" stroke="currentColor" stroke-width="2.66667" stroke-linecap="round" stroke-linejoin="round"/></svg></a>
          : <span class="disabled"><svg aria-hidden="true" width="32" height="32" viewBox="0 0 32 32" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M24.6667 9L18 15.6667L24.6667 22.3333" stroke="currentColor" stroke-width="2.66667" stroke-linecap="round" stroke-linejoin="round"/><path d="M14.6667 9L8 15.6667L14.6667 22.3333" stroke="currentColor" stroke-width="2.66667" stroke-linecap="round" stroke-linejoin="round"/></svg></span>
      }
</li>

After formatting...

 <li>
      {
        firstPage ? (
          <a href={firstPage} aria-label="Go to the first page">
            <svg aria-hidden="true" width="32" height="32" viewBox="0 0 32 32" fill="none" xmlns="http://www.w3.org/2000/svg">
              <>
                <path d="M24.6667 9L18 15.6667L24.6667 22.3333" stroke="currentColor" stroke-width="2.66667" stroke-linecap="round" stroke-linejoin="round" />
                <path d="M14.6667 9L8 15.6667L14.6667 22.3333" stroke="currentColor" stroke-width="2.66667" stroke-linecap="round" stroke-linejoin="round" />
              </>
            </svg>
          </a>
        ) : (
          <span class="disabled">
            <svg aria-hidden="true" width="32" height="32" viewBox="0 0 32 32" fill="none" xmlns="http://www.w3.org/2000/svg">
              <>
                <path d="M24.6667 9L18 15.6667L24.6667 22.3333" stroke="currentColor" stroke-width="2.66667" stroke-linecap="round" stroke-linejoin="round" />
                <path d="M14.6667 9L8 15.6667L14.6667 22.3333" stroke="currentColor" stroke-width="2.66667" stroke-linecap="round" stroke-linejoin="round" />
              </>
            </svg>
          </span>
        )
      }
    </li>

@markteekman
Copy link
Member

markteekman commented Apr 4, 2023

Hey @david-abell! Thanks for putting in the time, even if you've got nothing but free time right now 😉 Adding prettier is a good idea though! And to be honest, the formatting of the pagination component seems quite alright, you could argue it's more readable this way. Only thing I don't understand is the <> and </> tags wrapping the paths. And I don't think I've got any local settings of prettier, it should all be in the .prettierrc file 🤔

@david-abell
Copy link
Contributor

Hey @markteekman, I hadn't noticed the empty tags but they must be a reformatting artifact. I only see them in that one component. Maybe the custom formatting is because your editor settings don't automatically format *.astro files. I noticed VScode wasn't formatting on my end either until adding prettier-plugin-astro to the project.

This is sort of embarrassing but I seem to have posted a longer line limit test when I posting my above example instead of the .prettierrc output from the accessible astro starter... Oops. Single line svgs are the only thing causing major line changes though so its just the pagination component that got hit with most of the changes.

I'll get a pull request sorted to review.

The reformatted svg example I actually meant to post as an example is this...

      {
        previousPage ? (
          <a href={previousPage} aria-label={`Go back to ${previousPage}`}>
            <svg xmlns="http://www.w3.org/2000/svg" aria-hidden="true" width="32" height="32" viewBox="0 0 24 24">
              <path
                fill="none"
                stroke="currentColor"
                stroke-linecap="round"
                stroke-linejoin="round"
                stroke-width="2"
                d="m14 7-5 5 5 5"
              />
            </svg>
          </a>
        ) : (
          <span class="disabled">
            <svg xmlns="http://www.w3.org/2000/svg" aria-hidden="true" width="32" height="32" viewBox="0 0 24 24">
              <path
                fill="none"
                stroke="currentColor"
                stroke-linecap="round"
                stroke-linejoin="round"
                stroke-width="2"
                d="m14 7-5 5 5 5"
              />
            </svg>
          </span>
        )
      }

@markteekman
Copy link
Member

Great @david-abell! Adding the prettier-plugin-astro makes sense, I forgot about that one 😅 I have some time to look at your PR, so I'll get on it right away!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants