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: drop @types/cheerio #36

Merged
merged 1 commit into from
Nov 24, 2023
Merged

refactor: drop @types/cheerio #36

merged 1 commit into from
Nov 24, 2023

Conversation

yunsii
Copy link
Contributor

@yunsii yunsii commented Nov 18, 2023

#35

@yunsii
Copy link
Contributor Author

yunsii commented Nov 18, 2023

Export default cheerio instance is deprecated, ref: https://github.com/cheeriojs/cheerio/blob/v1.0.0-rc.12/src/index.ts#L64

So importing * as cheerio instead of this default export, ref: https://github.com/cheeriojs/cheerio/blob/a13961b99f11fd260e6594f995ad86759347eddb/src/index.ts#L70 (newest code for now)

@cyberalien cyberalien changed the base branch from main to next November 18, 2023 07:11
@yunsii yunsii marked this pull request as ready for review November 23, 2023 08:24
@cyberalien
Copy link
Member

Tested, it fails to compile pretty much on every change.

Are you not getting any errors when trying to build it?

export type CheerioElement = cheerio.TagElement;
export type WrappedCheerioElement = cheerio.Cheerio;
export type CheerioElement = cheerio.Element;
export type WrappedCheerioElement = cheerio.Cheerio<cheerio.Element>;
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't work, throws error.

@@ -75,7 +77,7 @@ interface ExtendedTagElementRelations {
* Extended tag
*/
export interface ExtendedTagElement
extends cheerio.TagElement,
extends cheerio.Element,
Copy link
Member

Choose a reason for hiding this comment

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

Why? And it doesn't work.

@yunsii
Copy link
Contributor Author

yunsii commented Nov 24, 2023

I only run build and test of @iconify/tools, I will process it right now.

@yunsii
Copy link
Contributor Author

yunsii commented Nov 24, 2023

@cyberalien It's wired, why encounter these errors?
image

@cyberalien
Copy link
Member

I only run build and test of @iconify/tools, I will process it right now.

Yes, I'm running that too, other packages don't really matter. It doesn't work for me.

@cyberalien It's wired, why encounter these errors? image

This means dependencies weren't installed. Re-run pnpm install (or ni if you are using @antfu/ni package).

@cyberalien
Copy link
Member

I only run build and test of @iconify/tools, I will process it right now.

Yes, I'm running that too, other packages don't really matter. It doesn't work for me.

I'm getting errors when parsing types on every changed file.

@yunsii
Copy link
Contributor Author

yunsii commented Nov 24, 2023

I'm getting errors when parsing types on every changed file.

What's the meaning? Here is my result:

image

Build passed, test encounters download errors only.

@cyberalien
Copy link
Member

Strange. Do you get errors when running lint command?

@yunsii
Copy link
Contributor Author

yunsii commented Nov 24, 2023

Strange. Do you get errors when running lint command?

image

It seems alse not.

@cyberalien
Copy link
Member

This is what I get with build, with lint and you can also see a warning in editor

Screenshot 2023-11-24 at 12 24 45
Screenshot 2023-11-24 at 12 24 37

@cyberalien
Copy link
Member

This is a very weird situation, not sure which dev environment is bugged: maybe yours, maybe mine. So currently running ci to test on separate environment.

@cyberalien
Copy link
Member

So ci run was a success, which means bug is in my environment. Very very weird.

Doh. Sorry.

@yunsii
Copy link
Contributor Author

yunsii commented Nov 24, 2023

I think you can use a online IDE to test, it works in my personal and company computer.

@cyberalien
Copy link
Member

Got it to work!

It appears pnpm left @types/cheerio in modules, even though packages no longer have it.

Doing some tests now...

@cyberalien
Copy link
Member

Works perfectly

@cyberalien cyberalien merged commit 4cadec0 into iconify:next Nov 24, 2023
1 check passed
@cyberalien
Copy link
Member

Thanks a lot and sorry for mess.

Merged. I've also removed duplicate entries, so types are now in one file.

@cyberalien
Copy link
Member

Published with next tag.

For now it won't be added to stable because:

  • This is likely a breaking change, so at least new minor version is needed, not patch.
  • I've ran into unrelated issue when working with large icons, which will more likely require another breaking change to fix, but that one will be a major change.

So this is a possibly breaking change and I will likely add another breaking change soon, so I'd rather have one new major version with all those changes instead of publishing temporary version.

@yunsii
Copy link
Contributor Author

yunsii commented Nov 24, 2023

Sure, @types/cheerio's types declare in global namespace 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants