Skip to content

fix: remove deprecated package @types/sass #583

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

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

tlaundal
Copy link
Contributor

@tlaundal tlaundal commented Mar 5, 2023

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

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to run pnpm lint!)

Tests

  • Run the tests with npm test or pnpm test

@Conduitry
Copy link
Member

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
Copy link
Contributor Author

tlaundal commented Mar 5, 2023

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
Copy link
Member

dummdidumm commented Mar 6, 2023

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
Copy link
Contributor Author

tlaundal commented Mar 6, 2023

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
Copy link
Member

kaisermann commented Mar 14, 2023

@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 kaisermann self-requested a review March 14, 2023 17:09
@kaisermann kaisermann merged commit 731516d into sveltejs:main Mar 14, 2023
@kaisermann
Copy link
Member

kaisermann commented Mar 14, 2023

Released in v5.0.2 🎉

@tlaundal tlaundal deleted the remove-deprecated-sass-types branch March 14, 2023 17:22
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.

@types/sass deprecated
4 participants