sveltejs / svelte-preprocess

A ✨ magical ✨ Svelte preprocessor with sensible defaults and support for: PostCSS, SCSS, Less, Stylus, Coffeescript, TypeScript, Pug and much more.
MIT License
1.73k stars 147 forks source link

fix: remove deprecated package @types/sass #583

Closed tlaundal closed 1 year ago

tlaundal commented 1 year ago

Fixes #582.

My best guess is that @types/sass was included in dependencies (and not devDependencies) as a convenience for users. With types now being included in the main sass package, it should no longer be an issue that people might have sass without the types, so we can safely remove the types here without promoting sass from devDependencies to dependencies.

This removes the warning described in #582.

Before submitting the PR, please make sure you do the following

Tests

Conduitry commented 1 year ago

This is probably a good change to make, but it should probably also accompany a bump of the peerdep on Sass (to when the types are included in the package), which would necessitate a major bump - so we should try to see what other breaking changes we should batch that with.

tlaundal commented 1 year ago

Hmm, fair point. We require version ^1.26.8 as peer, while the types were added in version 1.45.0.

I'm not quite sure if type declarations are resolved in the same way as normal modules, but if they are then including @types/sass as a dependency of this package is not enough to ensure it is available for the users of the package. This is likely a moot point since it probably works in 90% of cases, though.

Would you like me to add the bump for the peer dep in this PR?

dummdidumm commented 1 year ago

I don't see any mention of @types/sass anywhere in the code base, so I'm wondering if this is just a leftover and we can just remove it. @kaisermann do you have more info on why this is added as a dependency?

tlaundal commented 1 year ago

I don't see any mention of @types/sass anywhere in the code base, so I'm wondering if this is just a leftover and we can just remove it. @kaisermann do you have more info on why this is added as a dependency?

Any import of sass would use these types, at least before sass included its own types. At least some of them show up with a search.

I can't speak for the original intention, but the dependency was introduced here, and it seems very much deliberate that only a few of the new types were included as dependencies, with the rest being only dev dependencies. My best guess is it was to ensure the types exported by this package would be valid in projects that import it.

This is also the breakage Conduitry is referencing, that the type check for a project would break because they are on a version of sass that doesn't export the types, and they are no longer included as a dependency by svelte-preprocess.

kaisermann commented 1 year ago

@kaisermann do you have more info on why this is added as a dependency?

As @tlaundal rightly said, it was for the consumer's convenience as the options object would be typed, etc.

CleanShot 2023-03-14 at 18 14 39

If I'm not forgetting anything, we can keep the current sass peer version and remove the @types/sass dependency without any major issues. I think the worst that can happen is someone using a version below 1.45.0 trying to pass some sass configs without auto-completion.

If something unexpected happens, we can rollback this patch release and follow what @Conduitry suggested

kaisermann commented 1 year ago

Released in v5.0.2 🎉