Skip to content

refactor: remove deprecated Sass wrapper macros#7103

Open
cdavalos7 wants to merge 2 commits intotensorflow:masterfrom
cdavalos7:feature/remove-sass-wrapper-deprecated
Open

refactor: remove deprecated Sass wrapper macros#7103
cdavalos7 wants to merge 2 commits intotensorflow:masterfrom
cdavalos7:feature/remove-sass-wrapper-deprecated

Conversation

@cdavalos7
Copy link
Copy Markdown
Contributor

@cdavalos7 cdavalos7 commented May 7, 2026

Motivation for features / changes

Unblock internal LSC intended to remove workarounds for Sass and bad practices. Internal issue: b/508363660. This PR removes the wrappers for Sass build rules. Sass rules must directly declare their dependencies.

Per Sass build rules guidelines, wrapper macros are discouraged, and tf_external_sass_libray was relying on a loophole (passing a sass_library to another sass_library's srcs) that is being closed.

Technical description of changes

  • Replace tf_sass_binary,tf_sass_library, and tf_external_sass_libray wrappers with direct calls to sass_binary, sass_library, and npm_sass_library from @io_bazel_rules_sass.
  • Updated hash version rules_sass from 1.55.0 to 1.69.5
  • Replaces the angular_material_sass_deps target with a direct npm_sass_library call. This produces a SassInfo provider consumed via deps, complying with the rule that sass libraries must declare deps directly rather than passing one library to another's srcs.
  • Added include_paths = ["external/npm/node_modules"] to each sass_binary so dart-sass can resolve @use '@angular/material'. The old wrapper set this automatically, now it has to be set per target.

Detailed steps to verify changes work correctly (as executed by you)

  • BUILD
  • Standalone running
  • No console errors

cdavalos7 added 2 commits May 7, 2026 16:29
…_sass_library, tf_sass_library and tf_sass_lbinary with direct calls to sass_binary, sass_library, and npm_sass_library from @io_bazel_rules_sass.
@cdavalos7 cdavalos7 marked this pull request as ready for review May 8, 2026 17:35
@cdavalos7 cdavalos7 changed the title Replaced wrapped of Angular materials in Bazel. Replacing tf_external… refactor: remove deprecated Sass wrapper macros May 8, 2026
@cdavalos7 cdavalos7 requested a review from arcra May 8, 2026 17:35
Comment thread tensorboard/defs/defs.bzl
TensorBoard has to depdend on them very specifically. This rule allows SASS
modules in NPM packages to be built properly.
"""
npm_sass_library(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can also remove the "load" statement at the top for these rules.

Comment thread tensorboard/webapp/BUILD
)

tf_external_sass_libray(
npm_sass_library(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this rule is not available internally. We might need to either in-line this dependency wherever it's used, or modify this to be a plain "sass_library", which AFAIU would require creating a file that @forwards whatever is used from this dependency.

The downside of in-lining it everywhere would be that if at some point we need or want another common dependency everywhere where this one is used, then we'd have to do that manually, or recreate this wrapper at that time and update all references anyway.

This sass_library wrapper would be useful to not have to update every place where this is currently used in that case, although I think that's somewhat unlikely.

Up to you, either way is fine by me.

Comment thread WORKSPACE
# rules_sass release information is difficult to find but it does seem to
# regularly release with same cadence and version as core sass.
# We typically upgrade this library whenever we upgrade rules_nodejs.
# The version used here is the most recent release as of 2023-07-11, which is the same version used by rules_nodejs 5.8.1.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version used here is the most recent release as of 2023-07-11

I think it's uncommon to say most recent as of <a date in the past>, although perhaps technically correct (?).

Can we rephrase to say something like "This version was the latest one available when rules_nodejs 5.8.1 was released, so we use it because we know they're compatible", or "This version is used by rules_nodejs 5.8.1", or something like that.

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