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

[core] fix: remove implicit dependency on colors package Sass #4891

Merged
merged 2 commits into from
Sep 3, 2021

Conversation

adidahiya
Copy link
Contributor

Summary of change

#4858 introduced a bug involving an implicit Sass dependency on @blueprintjs/colors for users of the core package. This meant that any one importing files like this (which, turns out, does happen, since we keep some sort-of useful variables there):

@import "~@blueprintjs/core/src/components/button/common";
@import "~@blueprintjs/core/src/components/menu/common";

would experience errors in their Sass build, especially with package managers like PNPM which do not allow such implicit dependencies. Blueprint users should not have to consume the @blueprints/colors directly unless they need the colors in JS.

I think we should have only moved the color hex strings in colors.ts in #4858, not also colors.scss. We don't have any consumers of @blueprintjs/colors/lib/colors... so it's not necessary at the moment. But now that I've released v1.0.0, there's not much I can do without a breaking change...

Reviewers should focus on:

Should I just bite the bullet and remove the duplicated colors.scss in @blueprintjs/colors, and release a major version bump (we could go to v2.0 or v3.0, the latter makes it match current BP version better)?

Screenshot

N/A

@adidahiya
Copy link
Contributor Author

Should I just bite the bullet and remove the duplicated colors.scss in @blueprintjs/colors, and release a major version bump (we could go to v2.0 or v3.0, the latter makes it match current BP version better)?

I decided to do just this. I don't expect anyone to use the Sass color variables from the standalone package; everyone is already importing @blueprintjs/core/lib/variables already. I will release this breaking change in @blueprints/colors v3.0.

FYSA @patrickszmucer

@blueprint-bot
Copy link

remove duplicate colors.scss

Previews: documentation | landing | table

@adidahiya adidahiya merged commit a52692e into develop Sep 3, 2021
@adidahiya adidahiya deleted the ad/fix-colors-dependency branch September 3, 2021 21:27
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.

2 participants