Repository navigation
CSS Variable Namespacing #67362
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
CSS Variable Namespacing #67362
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1130,13 +1130,36 @@ const _cssCommaInPlaceholderReGlobal = new RegExp(COMMA_IN_PLACEHOLDER, 'g'); | |
| const _cssSemiInPlaceholderReGlobal = new RegExp(SEMI_IN_PLACEHOLDER, 'g'); | ||
| const _cssColonInPlaceholderReGlobal = new RegExp(COLON_IN_PLACEHOLDER, 'g'); | ||
|
|
||
| // Matches `/* DISABLE_NAMESPACING */` optionally followed by any characters | ||
| // that are not a semicolon or closing brace (e.g. whitespace or a property name), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Semicolon, closing brace, or opening brace. |
||
| // up until it hits a CSS variable `--some-name`. | ||
| const _cssNamespaceRe = new RegExp( | ||
| String.raw`(?:(/\*\s*DISABLE_NAMESPACING\s*\*/)[^;{}]*?)?(--[a-zA-Z0-9_-]+)`, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Voicing my preference for I also think using a comment as a pragma will make big blocks of variable references uglier if every other line requires a comment node. This certainly isn't a hill I'll die on and I understand the concern of having variable references not syntactically matching their declaration (if declared in a global stylesheet), but I feel like the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to be clear, you're envisioning that I don't mind the This can also be confusing if I want to find usages of I agree
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm happy to run this by the broader team just to get general opinions on the syntax.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally hear you on the concern for tooling. I'm definitely not in love with either approach, and I'm happy to go with whatever the team prefers.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Closing the loop here, turns out We considered a few options, but ultimately landed on :host {
--global--foo: red;
color: var(--global--foo);
}
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI, attribute and class names can contain
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AGENT: The current regex
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels unlikely to be a big problem imo. One more reason I'm investigating using a real parser, though 👀 |
||
| 'g', | ||
| ); | ||
|
|
||
| export class CssRule { | ||
| constructor( | ||
| public selector: string, | ||
| public content: string, | ||
| ) {} | ||
| } | ||
|
|
||
| /** | ||
| * Transforms CSS variables within a stylesheet to include a namespace placeholder. | ||
| * | ||
| * E.g. `--foo: bar;` becomes `--%NS%foo: bar;` | ||
| * E.g. `color: var(--foo);` becomes `color: var(--%NS%foo);` | ||
| * | ||
| * If a declaration or usage is preceded by `/* DISABLE_NAMESPACING *\/`, it is NOT transformed. | ||
| */ | ||
| export function namespaceCssVariables(cssText: string): string { | ||
| return cssText.replace(_cssNamespaceRe, (match, disableComment, varName) => { | ||
| if (disableComment) return match; | ||
| return `--%NS%${varName.substring('--'.length)}`; | ||
|
dgp1130 marked this conversation as resolved.
Resolved
|
||
| }); | ||
| } | ||
|
|
||
| export function processRules(input: string, ruleCallback: (rule: CssRule) => CssRule): string { | ||
| const escaped = escapeInStrings(input); | ||
| const inputWithEscapedBlocks = escapeBlocks(escaped, CONTENT_PAIRS, BLOCK_PLACEHOLDER); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| /** | ||
| * @license | ||
| * Copyright Google LLC All Rights Reserved. | ||
| * | ||
| * Use of this source code is governed by an MIT-style license that can be | ||
| * found in the LICENSE file at https://angular.dev/license | ||
| */ | ||
|
|
||
| import {inject, Injectable} from '@angular/core'; | ||
| import {CSS_VAR_NAMESPACE} from './dom_renderer'; | ||
|
|
||
| /** | ||
| * A service that can be used to manually namespace CSS variable names at runtime. | ||
| * This is useful when reading or setting CSS variables dynamically in JavaScript that | ||
| * were transformed by the compiler during the build. | ||
| * | ||
| * @publicApi | ||
| */ | ||
| @Injectable({providedIn: 'root'}) | ||
| export class CssVarNamespacer { | ||
| private readonly namespacePrefix = inject(CSS_VAR_NAMESPACE, {optional: true}) ?? ''; | ||
|
|
||
| /** | ||
| * Prepends the namespace prefix to a CSS variable name. | ||
| * | ||
| * @param name The CSS variable name to namespace, including the leading `--`. | ||
| * @returns The namespaced CSS variable name, including the leading `--`. Returns the input | ||
| * unchanged if no namespace is configured. | ||
| */ | ||
| namespace(name: string): string { | ||
| // Validate that the whole `--foo` variable is passed in. | ||
| if (typeof ngDevMode === 'undefined' || ngDevMode) { | ||
| if (!name.startsWith('--')) { | ||
| throw new Error( | ||
| `CSS variable names passed to \`CssVarNamespacer\` must start with '--', got: '${name}'`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // We want to support libraries which might be used by applications which do and don't | ||
| // namespace variables. Therefore the library always needs to use `CssVarNamespacer`, even | ||
| // though the application may not actually be namespacing anything. | ||
| if (!this.namespacePrefix) return name; | ||
|
|
||
| return `--${this.namespacePrefix}${name.substring('--'.length)}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies if we've talked about this already and I'm forgetting, but have we considered making this a build-time transform rather than compile-time? Could we even just generate a purely unique ID at build time and use it for the entire app, then skip the runtime replacement with
%NS%entirely?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you mean "rather than runtime"? I did look into this, and it would probably work in the 3P environment. However, in 1P we have a few extra constraints.
DISABLE_NAMESPACING), so we can't enable this unconditionally, we need some kind of opt-in, whether that's a compiler flag or a runtime flag.enable_css_namepsacing = "my-app"on everyng_module(which would break libraries used by multiple apps who can't choose a single namespace) or you need a Bazel transition and the build story gets a lot more complicated.I do agree with the general desire that this should be done at build time, but practically speaking that makes the feature much more complicated and I think it would be unlikely to be feasible in google3 based on the way our build tooling works. I'm inclined to start with a runtime transform and then potentially switch to a build-time transform later if/when we have a clearer need and path towards it in google3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
No dispute with the need for a flag.
Does the namespace need to be the app/binary ID, or is any ID sufficient so long as it's unique to a single build of the app? Maybe we could just use an ID we generate at build time?
I agree that this is the tricky part. I'm not sure how we'd define a custom ID in g3 that would be available across the whole build. You're much more knowledgable here than I am, so I trust your thoughts on it.
Seems totally reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MicA has to account for SSR-ing two versions of the same application at the same time while in the middle of a deployment (one request happens to go to app version X, another request happens to go app version X + 1). In that scenario, I don't think any information known at build time would be sufficient. That's a very MicA-specific concern, but it's the constraints we're working within.
I forgot to link to the relevant part of the internal doc which discussed this. A transition is definitely plausible (we did it for Ivy), but also very much complexity we'd rather avoid. If we receive sufficient push back from g3 customers, then that's a sign that the performance impact is significant enough to consider re-evaluating the complexity here.