twolfson / spritesmith

Utility that takes sprites and converts them into a stylesheet and its coordinates
MIT License
916 stars 56 forks source link

[Proposal] Upgrade Dependencies and Rewrite it with TypeScript #94

Closed YunYouJun closed 9 months ago

YunYouJun commented 9 months ago

spritesmith depends on a very old semver@5.

semver@7 has a breaking change. When we use it with other packages that rely on semver, it will report an error. The same goes for LRU.

So I want to upgrade semver.

Also, smith doesn't have much code, I think we can rewrite it with ts so we don't need to install @types/spritesmith as an extra.


If you approve but don't have time to fix it, please let me know and I'll be happy to create a PR.

twolfson commented 9 months ago

Thanks for writing in! Upgrading semver sounds great!

I was actually surprised we even use it, but I see it's for engine checking

I don't have a lot of bandwidth so a PR is very welcome =)

YunYouJun commented 9 months ago

By the way, phantomjs is a discontinued headless browser used for automating web page interaction.

https://en.wikipedia.org/wiki/PhantomJS

Should we use playwright?

twolfson commented 9 months ago

The phantomjssmith engine is deprecated as well ;)

I think virtually everyone uses pixelsmith as the underlying engine, since that's the default. But I kind of like leaving it configurable for those who want/need more

EDIT: To answer your question, no need to touch any dependencies/code outside of semver

YunYouJun commented 9 months ago

After I upgraded semver, it didn't solve the underlying problem because it relied on lru-cache@6.0 and the latest lru-cache version was 10.x.

Actually spritesmith only uses the semver.satisfies function, can we add a build system to package the function and publish the dist files? Users do not need to install the semver package.

twolfson commented 9 months ago

Ah, that's unfortunate =(

I'm not really a fan of adding build systems to Node.js focused packages. It significantly complicates in-field troubleshooting and future maintenance =/

Have you tried asking semver to upgrade? iirc I initially chose them because they were used by npm

Otherwise, the simplest path forward is prob just having you fork spritesmith to remove the versioning.

I'd prefer to not drop the logic in the main package, as it seems some folks still use different engines still:

For reference, semver is 256M weekly downloads

YunYouJun commented 9 months ago

Okay, I understand your concern. My scenario is because I'm using it in an electron program that dynamically requires semver, and semver continues to dynamically require lru-cache@6, so it conflicts with the lru-cache@10 API that my other programs dynamically require.

Maybe my situation is special, so I will fork spritesmith to make some changes.

Thank you for your prompt reply and have a great day.