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.

Sign up for GitHub

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

Jump to bottom

[Bug]: [email protected] Mixed Declarations deprecation warning in @carbon/react #16962

Open
62 tasks
0xFA11 opened this issue Jul 15, 2024 · 8 comments
Open
62 tasks

[Bug]: [email protected] Mixed Declarations deprecation warning in @carbon/react #16962

0xFA11 opened this issue Jul 15, 2024 · 8 comments
Assignees
@Gururajj77
Labels
package: @carbon/react @carbon/react planning: umbrella Umbrella issues, surfaced in Projects views role: dev 🤖 severity: 3 https://ibm.biz/carbon-severity type: bug 🐛 type: infrastructure 🤖 Issues relating to devops, tech debt, etc.
Milestone
2024 Q4

Comments

@0xFA11
Copy link

0xFA11 commented Jul 15, 2024 •
edited by tay1orjones
Loading

Sass recently included a new deprecation warning for mixed declarations.

This causes very noisy console output when working on this repo, as well as consumer environments, including the stackblitz sandboxes.

Important

This is just a warning and will not fail a build unless specifically configured to fail on warning messages.

Click to expand original issue body text

Package

@carbon/react

Browser

Chrome

Package version

v1.61.0

React version

v18.2.0

Description

Sass 1.77.7 recently introduced a deprecation which causes @carbon/react to fail on Vite with React18.2.0

https://github.com/sass/dart-sass/blob/main/CHANGELOG.md#1777

1.77.7
Declarations that appear after nested rules are deprecated, because the semantics Sass has historically used are different from the semantics specified by CSS. In the future, Sass will adopt the standard CSS semantics.

Also: https://sass-lang.com/documentation/breaking-changes/mixed-decls/

Breaking Change: Mixed Declarations
CSS is changing the way it handles declarations mixed with nested rules, and we want to make sure Sass matches its behavior.
(...)

Reproduction/example

https://stackblitz.com/edit/github-2vsntp?file=package.json

Steps to reproduce

simply install [email protected] to use with @carbon/react

Suggested Severity

Severity 2 = User cannot complete task, and/or no workaround within the user experience of a given component.

Application/PAL

No response

Code of Conduct

Below is the scope of work for this:

Utility files to update
Beta Give feedback

Component files to update
Beta Give feedback
@tay1orjones
Copy link
Member

tay1orjones commented Jul 15, 2024 •
edited
Loading

@ThusSpokeNomad Thanks! We just noticed too from an internal report of the same error.

We'll work on fixing this, but in the meantime you can ignore the warning, silence the warning or downgrade sass if you'd prefer.

@tay1orjones tay1orjones added the severity: 3 https://ibm.biz/carbon-severity label Jul 15, 2024
@tay1orjones tay1orjones changed the title [Bug]: Heads-up! [email protected] breaks @carbon/react [Bug]: [email protected] Mixed Declarations deprecation warning in @carbon/react Jul 15, 2024
@tay1orjones tay1orjones added type: infrastructure 🤖 Issues relating to devops, tech debt, etc. and removed status: needs triage 🕵️‍♀️ labels Jul 15, 2024
@tay1orjones
Copy link
Member

tay1orjones commented Jul 17, 2024 •
edited
Loading

This will probably need to turn into an umbrella issue - I believe we have 1000+ instances we need to fix. Breaking it out per-component will probably make the most sense to keep the PR size manageable/reviewable for each.

This can be a slow-burn lower priority item over multiple sprints as it's just a warning that can be suppressed.

@marcellobarile
Copy link

marcellobarile commented Jul 18, 2024 •
edited
Loading

just a warning that can be suppressed

do you know starting from which version of Saas this will become a real problem?
I foresee the impossibility of upgrading an existing solution if you don't tackle it promptly.

@0xFA11
Copy link
Author

0xFA11 commented Jul 18, 2024

just a warning that can be suppressed

do you know starting from which version of Saas this will become a real problem? I foresee the impossibility of upgrading an existing solution if you don't tackle it promptly.

@marcellobarile, as in the description, starting from 1.77.7

@marcellobarile
Copy link

as in the description, starting from 1.77.7

if I got it right with 1.77.7 they have introduced the deprecation warning; I was wondering when the deprecations will not be supported anymore.
But they state In a future release, Dart Sass will change to match the ordering produced by plain CSS nesting. hence there might not be a milestone yet, nonetheless I hope this will be tackled on time.

@AlanGreene
Copy link

From https://sass-lang.com/documentation/breaking-changes/

Dart Sass will emit deprecation warnings for at least three months before releasing a breaking change, and will release the breaking change with a new major version number unless that change is necessary for CSS compatibility. CSS compatibility changes are often both non-disruptive and time-sensitive, so they may be released with new minor version numbers instead.

So it could come in a minor bump (e.g. 1.78.x or later), or a major bump (2.x), depending on how they choose to apply the rules in this case.

Disclaimer: I have no insight into the decision or plans, I'm just an interested party.

This was referenced Aug 12, 2024
Closed
Merged
@tay1orjones tay1orjones added the package: @carbon/react @carbon/react label Aug 21, 2024
@tay1orjones
Copy link
Member

tay1orjones commented Aug 21, 2024 •
edited
Loading

I put together a list of files that need to be updated. I pulled down #17141, updated the css build task to use the verbose log level, piped all the ~1100 deprecation notices we have right now into a file, cut it down to just the file path listed in each, and then deduped that list. I've updated the issue body to contain the list and have the typical format of one of our umbrella issues.

@tay1orjones tay1orjones added the planning: umbrella Umbrella issues, surfaced in Projects views label Aug 21, 2024
@OmranAbazid OmranAbazid mentioned this issue Aug 23, 2024
Merged
3 tasks
ioV9x pushed a commit to ioV9x/mytfgames that referenced this issue Aug 24, 2024
build: Update sass to v1.77.6
db1b48b 
We won't upgrade further than v1.77.6 until carbon fixes their mixed
declarations to match CSS rules.

Refs: carbon-design-system/carbon#16962
ioV9x added a commit to ioV9x/mytfgames that referenced this issue Aug 24, 2024
@ioV9x
build: Update sass to v1.77.6
f0da53e 
We won't upgrade further than v1.77.6 until carbon fixes their mixed
declarations to match CSS rules.

Refs: carbon-design-system/carbon#16962
ioV9x added a commit to ioV9x/mytfgames that referenced this issue Aug 24, 2024
@ioV9x
build: Update sass to v1.77.6
f54969c 
We won't upgrade further than v1.77.6 until carbon fixes their mixed
declarations to match CSS rules.

Refs: carbon-design-system/carbon#16962
@tay1orjones tay1orjones added this to the 2024 Q3 milestone Sep 13, 2024
@sstrubberg sstrubberg modified the milestones: 2024 Q3, 2024 Q4 Sep 16, 2024
@tay1orjones tay1orjones mentioned this issue Sep 18, 2024
Closed
2 tasks
@tay1orjones
Copy link
Member

Approach discussion notes

  1. Add the & to declarations that are currently below/after the mixin, which results in the stylelint error of duplicate declarations. If we ignore that, we're taking on the debt of having to later refactor those into a single declaration and ensuring that any styles that previously came after a mixin being moved to above the mixin do not introduce regressions
  • This is the easy way to get rid of the warnings
  • Stylelint may have some movement on updating the rule for this situation
  1. Move the styles up above the mixin in the same declaration. The risk here is introducing visual bugs.
  • This is more difficult (and risky due to potential visual bugs) but we don't have any tech debt from it
  • More visual bugs is worse than these deprecation warnings that don't really matter right now
  • We can move slow enough on this to do it right the first time
  • These PRs are easily revertable
  • Why would you have an overriding style coming after a mixin anyway? There isn't a valid reason for this (probably), but if there is we implement the first approach on a case by case basis (and have way less stylelint ignore blocks, which was 18-20 for accordion, a heavier component 50)

Go for 2, if needed use the approach of 1 on a case by case basis. We need to document this for community members to understand more clearly what we're aiming for.

smaller components a 3, larger a 5 - size all initially as a 5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Gururajj77 Gururajj77

Labels
package: @carbon/react @carbon/react planning: umbrella Umbrella issues, surfaced in Projects views role: dev 🤖 severity: 3 https://ibm.biz/carbon-severity type: bug 🐛 type: infrastructure 🤖 Issues relating to devops, tech debt, etc.
Projects
Design System
Status: 🪆 Needs Refined
Roadmap
Status: Next ➡
Milestone
2024 Q4
Development

Successfully merging a pull request may close this issue.

refactor: update scss files to follow new ruleset for nested styles from 1.77.7 Gururajj77/carbon
6 participants
@marcellobarile @AlanGreene @tay1orjones @0xFA11 @sstrubberg @Gururajj77