refactor: remove deprecated Sass wrapper macros#7103
refactor: remove deprecated Sass wrapper macros#7103cdavalos7 wants to merge 2 commits intotensorflow:masterfrom
Conversation
…_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.
| TensorBoard has to depdend on them very specifically. This rule allows SASS | ||
| modules in NPM packages to be built properly. | ||
| """ | ||
| npm_sass_library( |
There was a problem hiding this comment.
I think we can also remove the "load" statement at the top for these rules.
| ) | ||
|
|
||
| tf_external_sass_libray( | ||
| npm_sass_library( |
There was a problem hiding this comment.
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.
| # 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. |
There was a problem hiding this comment.
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.
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
tf_sass_binary,tf_sass_library, andtf_external_sass_libraywrappers with direct calls tosass_binary,sass_library, andnpm_sass_libraryfrom@io_bazel_rules_sass.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)