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

refactor: Convert to TypeScript #1816

Merged
merged 3 commits into from
May 6, 2021
Merged

refactor: Convert to TypeScript #1816

merged 3 commits into from
May 6, 2021

Conversation

fb55
Copy link
Member

@fb55 fb55 commented Apr 16, 2021

Initial big lift to convert the codebase, broken into three commits to make the bigger changes obvious. Everything seems to work now, including detection if users pass selectors ($('<div>') will product a Cheerio<Node> type, $('.foo') a Cheerio<Element>).

Currently missing:

  • The ergonomics of using types aren't great. We should re-export all regularly used types.
  • We can now use typedoc to generate our documentation. Produced docs still have to be improved:
    • Examples now need to be put in code blocks.
    • The organization is very hard to grasp.
  • Benchmarks have to be updated to use ts-node.

@fb55 fb55 force-pushed the refactor/ts branch 13 times, most recently from 4ac66f4 to 15421df Compare April 19, 2021 00:02
@fb55 fb55 marked this pull request as ready for review April 19, 2021 00:02
@fb55 fb55 force-pushed the refactor/ts branch 5 times, most recently from 67314d7 to 24f1b92 Compare April 20, 2021 20:35
@voxpelli
Copy link

voxpelli commented May 7, 2021

Exciting stuff @fb55! Great work 🎉

"esModuleInterop": true
}
/* Basic Options */
"target": "es5" /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019' or 'ESNEXT'. */,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fb55 sorry for the CC on an old PR, but I didn't want to make a new issue just for this. Is there any reason you didn't go with ES2017 or whatever is equivalent for Node.js 10, which is the oldest supported Node.js version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, NVM I see now that the package.json engines is set to >=6. On CI Node.js >= 10 is tested hence why I thought this could be improved. IMHO, these two should be in sync, especially since Node.js 6 is ancient...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jest doesn't support Node 6, which is why Node 6 isn't part of the CI anymore.

I am still a bit hesitant to bump the Node version, primarily due to the discussion in #1585. But the mismatch between CI and package.json seems like a good reason to make the cut.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I remember, thanks for the info.

So, yeah, not sure what the proper semver way would be. Perhaps release v1.0.0 final with the current engines, and then cut a new v2.0.0 with the bump in engines and tsconfig.

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.

None yet

3 participants