-
-
Notifications
You must be signed in to change notification settings - Fork 5k
Extract used CSS variables from .css files
#17433
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
Changes from 5 commits
7dc7878
7b2a5c1
7eb4908
9f7d0f9
481793b
5272925
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| css | ||
| less | ||
| lock | ||
| sass | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,6 +84,9 @@ pub struct Scanner { | |
| /// All found extensions | ||
| extensions: FxHashSet<String>, | ||
|
|
||
| /// All CSS files we want to scan for CSS variable usage | ||
| css_files: Vec<PathBuf>, | ||
|
|
||
| /// All files that we have to scan | ||
| files: Vec<PathBuf>, | ||
|
|
||
|
|
@@ -212,11 +215,25 @@ impl Scanner { | |
| fn extract_candidates(&mut self) -> Vec<String> { | ||
| let changed_content = self.changed_content.drain(..).collect::<Vec<_>>(); | ||
|
|
||
| let candidates = parse_all_blobs(read_all_files(changed_content)); | ||
| // Extract all candidates from the changed content | ||
| let mut new_candidates = parse_all_blobs(read_all_files(changed_content)); | ||
|
|
||
| // Extract all CSS variables from the CSS files | ||
| let css_files = self.css_files.drain(..).collect::<Vec<_>>(); | ||
| if !css_files.is_empty() { | ||
| let css_variables = extract_css_variables(read_all_files( | ||
| css_files | ||
| .into_iter() | ||
| .map(|file| ChangedContent::File(file, "css".into())) | ||
| .collect(), | ||
| )); | ||
|
|
||
| new_candidates.extend(css_variables); | ||
| } | ||
|
|
||
| // Only compute the new candidates and ignore the ones we already have. This is for | ||
| // subsequent calls to prevent serializing the entire set of candidates every time. | ||
| let mut new_candidates = candidates | ||
| let mut new_candidates = new_candidates | ||
| .into_par_iter() | ||
| .filter(|candidate| !self.candidates.contains(candidate)) | ||
| .collect::<Vec<_>>(); | ||
|
|
@@ -248,6 +265,12 @@ impl Scanner { | |
| .and_then(|x| x.to_str()) | ||
| .unwrap_or_default(); // In case the file has no extension | ||
|
|
||
| // Special handing for CSS files to extract CSS variables | ||
| if extension == "css" { | ||
| self.css_files.push(path); | ||
| continue; | ||
| } | ||
|
Comment on lines
+268
to
+272
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. Why is this necessary? Wouldn't we literally want to emit this in the
Member
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. Yeah you are right I think. My thinking was that changing a CSS file triggers a full rebuild anyway. But that statement doesn't hold true for We also have to be careful that we don't scan and watch the
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. Mind following up on that? Just so we don't forget
Member
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. For future reference. Follow up PR: #17467 |
||
|
|
||
| self.extensions.insert(extension.to_owned()); | ||
| self.changed_content.push(ChangedContent::File( | ||
| path.to_path_buf(), | ||
|
|
@@ -402,6 +425,44 @@ fn read_all_files(changed_content: Vec<ChangedContent>) -> Vec<Vec<u8>> { | |
| .collect() | ||
| } | ||
|
|
||
| #[tracing::instrument(skip_all)] | ||
| fn extract_css_variables(blobs: Vec<Vec<u8>>) -> Vec<String> { | ||
|
Member
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 started passing options to the other
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. I feel like the only difference really should be that we no-op the
Member
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 started passing through the contents and the extension and it leaked everywhere. Can see that in this commit: 7dc7878 Then I was thinking about an So instead of checking it in this hot path, I created a separate function instead. |
||
| let mut result: Vec<_> = blobs | ||
| .par_iter() | ||
| .flat_map(|blob| blob.par_split(|x| *x == b'\n')) | ||
| .filter_map(|blob| { | ||
| if blob.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| let extracted = | ||
| crate::extractor::Extractor::new(blob).extract_css_variables_from_css_files(); | ||
| if extracted.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| Some(FxHashSet::from_iter(extracted.into_iter().map( | ||
| |x| match x { | ||
| Extracted::CssVariable(bytes) => bytes, | ||
| _ => &[], | ||
| }, | ||
| ))) | ||
| }) | ||
| .reduce(Default::default, |mut a, b| { | ||
| a.extend(b); | ||
| a | ||
| }) | ||
| .into_iter() | ||
| .map(|s| unsafe { String::from_utf8_unchecked(s.to_vec()) }) | ||
| .collect(); | ||
|
|
||
| // SAFETY: Unstable sort is faster and in this scenario it's also safe because we are | ||
| // guaranteed to have unique candidates. | ||
| result.par_sort_unstable(); | ||
|
|
||
| result | ||
| } | ||
|
|
||
| #[tracing::instrument(skip_all)] | ||
| fn parse_all_blobs(blobs: Vec<Vec<u8>>) -> Vec<String> { | ||
| let mut result: Vec<_> = blobs | ||
|
|
||
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.
Bit of a mouthful, open to suggestions. Note: this is completely internal API.
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.
extract_variables_from_cssif you want a shorter name but this is fine imoThere 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.
Ah that's nicer, and shorter, thanks!