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

SASS: Replace @import #21151

Closed
garrett opened this issue Oct 22, 2024 · 5 comments · Fixed by #21155
Closed

SASS: Replace @import #21151

garrett opened this issue Oct 22, 2024 · 5 comments · Fixed by #21155
Labels

Comments

@garrett
Copy link
Member

garrett commented Oct 22, 2024

Building currently shows warnings like this:

▲ [WARNING] Sass @import rules are deprecated and will be removed in Dart Sass 3.0.0.

I remember I worked with Katerina to try to remove @import a while ago, and it wasn't easy (which is why we're still using @import). There's supposedly an automatic migration now, but I'm guessing we'll hit the same problems we hit back then, where it isn't a 1:1 replacement.

This means we might need to completely refactor how our CSS is combined together... which could be a huge pain. We'd need to use the SASS @use and @forward rules instead.

Details @ https://sass-lang.com/documentation/breaking-changes/import/#automatic-migration

Note: This isn't about CSS @import, which SASS will always support, but the custom version of @import that SASS implements, which lets you import SASS files for inclusion (more like embedding, with dependency tress), instead of soft linking CSS files together on the browser-level.

@garrett garrett added the bug label Oct 22, 2024
@garrett
Copy link
Member Author

garrett commented Oct 22, 2024

For it to work properly, we'll also need to have PatternFly replace SASS @import uses as well. I'm not sure if that'll happen in the PF5 timeframe (it very likely won't, if it hasn't happened already), so we'll probably need to lock SASS down to the 2.x versions until upgrading to a compatible PF and also doing the work to migrate away from SASS 2.x @import.

In other words, this could be a big ball of busywork for no gain, just to keep up with the stack. 😒

At least SASS compiles to CSS, and CSS is what we ship, so it should be fine to lock down SASS to 2.x for now. So, for the meantime, I'm suggesting we:

  1. lock SASS down to 2.x
  2. disable the warnings for this
  3. look to see if the "automatic" migration indeed works without any side effects
  4. see what PatternFly has done or is doing

@garrett
Copy link
Member Author

garrett commented Oct 22, 2024

Oh, this is also going to affect SASS "global built-in function calls". I'm not sure if that will affect us. The @import changes definitely will... a lot.

@garrett
Copy link
Member Author

garrett commented Oct 22, 2024

More details on a recent blog entry:

https://sass-lang.com/blog/import-is-deprecated/

Disabling the warnings is supposed to be done with a --silence-deprecation=import to the sass command. I hope we can somehow represent that command line flag in our build system.

@garrett
Copy link
Member Author

garrett commented Oct 22, 2024

OK, it looks like SASS is at 1.80.3 currently.

https://www.npmjs.com/package/sass

The rule should be < 3. Unless 3 is coming out any time soon, I suggest we squash the error from spewing out to the console, lock down SASS to be < 3, and move forward like ostriches with our heads in the sand on this. (That is: Ignore it for now.)

It turns out PatternFly did deal with at least similar migration pain @ patternfly/patternfly#4096 (comment), where they even had to get the SASS main dev involved to help them migrate. But that was because of /, not @import.

Meanwhile, I'm looking at the SASS code in node_modules and see @import all over the place.

@garrett
Copy link
Member Author

garrett commented Oct 22, 2024

Yep, PatternFly currently has a PR for PF6 to adapt to a lot of breaking changes @ patternfly/patternfly#7150 — it looks like they've mainly migrated away from @import in a PR prior to this, however, unless GitHub is hiding some of that from the diff (as it famously does these days).

martinpitt added a commit to martinpitt/cockpit that referenced this issue Oct 23, 2024
sass 1.80 (and 2.x) introduces tons of "@import rules are deprecated".
These are unfixable on our side due to PatternFly using them
extensively.

We don't need any new sass features, so stick to 1.79.x. Explicitly not
ignore 3.x (which will drop @import) to get a reminder in the future to
try and clean this up again.

Fixes cockpit-project#21151
martinpitt added a commit that referenced this issue Oct 23, 2024
sass 1.80 (and 2.x) introduces tons of "@import rules are deprecated".
These are unfixable on our side due to PatternFly using them
extensively.

We don't need any new sass features, so stick to 1.79.x. Explicitly not
ignore 3.x (which will drop @import) to get a reminder in the future to
try and clean this up again.

Fixes #21151
martinpitt added a commit to martinpitt/cockpit-machines that referenced this issue Oct 29, 2024
Reverts commit ebdef70.

See cockpit-project/cockpit#21151
We already configured this in dependabot (commit fac7a38), but that
landed too late, after the dependabot run.
jelly pushed a commit to cockpit-project/cockpit-machines that referenced this issue Oct 29, 2024
Reverts commit ebdef70.

See cockpit-project/cockpit#21151
We already configured this in dependabot (commit fac7a38), but that
landed too late, after the dependabot run.
jelly added a commit to jelly/cockpit-podman that referenced this issue Nov 6, 2024
See cockpit-project/cockpit#21151
We already configured this in dependabot (commit fac7a3824b), but that
landed too late, after the dependabot run.
jelly added a commit to cockpit-project/cockpit-podman that referenced this issue Nov 6, 2024
See cockpit-project/cockpit#21151
We already configured this in dependabot (commit fac7a3824b), but that
landed too late, after the dependabot run.
SludgeGirl pushed a commit to Nykseli/cockpit that referenced this issue Nov 12, 2024
sass 1.80 (and 2.x) introduces tons of "@import rules are deprecated".
These are unfixable on our side due to PatternFly using them
extensively.

We don't need any new sass features, so stick to 1.79.x. Explicitly not
ignore 3.x (which will drop @import) to get a reminder in the future to
try and clean this up again.

Fixes cockpit-project#21151
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant