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

feat: add types for packages #677

Merged
merged 2 commits into from
Dec 18, 2023
Merged

feat: add types for packages #677

merged 2 commits into from
Dec 18, 2023

Conversation

Kikobeats
Copy link
Member

Closes #676

@Kikobeats
Copy link
Member Author

Can you help me review this PR? @KeithGillette @jakebailey @navya9singh 🙂

@KeithGillette
Copy link

Trying out this commit in our project only makes the typing issues worse, as we weren't getting the TS2307 error previously:

error TS2307: Cannot find module 'metascraper' or its corresponding type declarations.
error TS7016: Could not find a declaration file for module 'metascraper-author'. '/Upath/to/node_modules/metascraper-author/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/metascraper-author` if it exists or add a new declaration (.d.ts) file containing `declare module 'metascraper-author';`
error TS7016: Could not find a declaration file for module 'metascraper-image'. '/path/to/node_modules/metascraper-image/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/metascraper-image` if it exists or add a new declaration (.d.ts) file containing `declare module 'metascraper-image';`
error TS7016: Could not find a declaration file for module 'metascraper-date'. '/path/to/node_modules/metascraper-date/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/metascraper-date` if it exists or add a new declaration (.d.ts) file containing `declare module 'metascraper-date';`
error TS7016: Could not find a declaration file for module 'metascraper-description'. '/path/to/node_modules/metascraper-description/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/metascraper-description` if it exists or add a new declaration (.d.ts) file containing `declare module 'metascraper-description';`
error TS7016: Could not find a declaration file for module 'metascraper-title'. '/path/to/node_modules/metascraper-title/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/metascraper-title` if it exists or add a new declaration (.d.ts) file containing `declare module 'metascraper-title';`
error TS7016: Could not find a declaration file for module 'metascraper-url'. '/path/to/node_modules/metascraper-url/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/metascraper-url` if it exists or add a new declaration (.d.ts) file containing `declare module 'metascraper-url';`

@Kikobeats
Copy link
Member Author

Thanks @jakebailey for the suggestion; @KeithGillette can you test it now?

@jakebailey
Copy link

I am not quite sure how you're testing this commit out; you'd need to import the types into all of the other packages as well (as they all have their own types, it's not just one repo). Maybe that's what you're already doing.

@Kikobeats
Copy link
Member Author

Ok let's ship it to see if it's working better for users 🚀

@Kikobeats Kikobeats merged commit 8b8339c into master Dec 18, 2023
30 of 31 checks passed
@Kikobeats Kikobeats deleted the types branch December 18, 2023 09:01
@KeithGillette
Copy link

Same TS7016 error on all rules with 5.42.0.

@Kikobeats
Copy link
Member Author

@KeithGillette can you try to fix it at your end and make a PR? 🙏

@KeithGillette
Copy link

I'd like to help, @Kikobeats, but I won't have time to devote to looking at this for at least a week and furthermore, I have no experience in publishing NPM packages, so would just be casting about trying to fix this.

I will note that in looking at what you've done, I think the problem may be that the types you've created for each rule are simply not getting installed because you haven't included them in the package.json files array for each package. While the packages/metascraper/package.json specifies including everything in src in its files array, spot-checking the packages/metascraper-<rulename>/package.json reveals that their files arrays only contain index.js, which is the only file that shows up in the src folders for me after installing the 5.42.0 rules.

@Kikobeats
Copy link
Member Author

@KeithGillette no worries, that's actually a good catch.

I fixed that in the last version published; Can you test it out?

@KeithGillette
Copy link

Better, but it looks like you missed adding types to metascraper-url altogether and I'm also still getting an error on metascraper-date since the path to the types in its package.json is scripts/index.d.ts instead of src/index.d.ts. I think the metascraper-date typings should perhaps be:

type Options = {
  /**
   * Whether to add the date published and date modified to the result.
   * @default true
   */
  datePublished?: boolean,
  /**
   * Whether to add the date published and date modified to the result.
   * @default true
   */
  dateModified?: boolean
}

export declare function rules(options?: Options): import('metascraper').Rules;

@Kikobeats
Copy link
Member Author

Thanks, fixed:

Can you test it? 🙂

@KeithGillette
Copy link

Latest release works for me, though we are only using the following rules:

import Metascraper from 'metascraper';
import MetascraperAuthor from 'metascraper-author';
import MetascraperImage from 'metascraper-image';
import MetascraperDate from 'metascraper-date';
import MetascraperDescription from 'metascraper-description';
import MetascraperTitle from 'metascraper-title';
import MetascraperURL from 'metascraper-url';

Thanks!

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.

Add types for metascraper packages
3 participants