microlinkhq / metascraper

Get unified metadata from websites using Open Graph, Microdata, RDFa, Twitter Cards, JSON-LD, HTML, and more.
https://metascraper.js.org
MIT License
2.35k stars 168 forks source link

feat: add types for packages #677

Closed Kikobeats closed 11 months ago

Kikobeats commented 11 months ago

Closes #676

Kikobeats commented 11 months ago

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

KeithGillette commented 11 months ago

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 commented 11 months ago

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

jakebailey commented 11 months ago

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 commented 11 months ago

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

KeithGillette commented 11 months ago

Same TS7016 error on all rules with 5.42.0.

Kikobeats commented 11 months ago

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

KeithGillette commented 11 months ago

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 commented 11 months ago

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

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

KeithGillette commented 11 months ago

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 commented 11 months ago

Thanks, fixed:

KeithGillette commented 11 months ago

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!