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

fixes #90 by adding postProcess hook to scoped-css-preprocessor to break cache for associated template files #95

Merged
merged 4 commits into from
Sep 18, 2023

Conversation

jakebixby
Copy link
Contributor

On css changes, the postProcess hook compares the previous template build to the current one using extracted classnames that get stored between builds. If there is a difference, we append a single space to the template file, which breaks the cache for the broccoli-persistent-filter that is processing the templates (ideally we would break the cache through the filter's mechanisms, but was unable to figure it out, so this hack works for now).

@changeset-bot
Copy link

changeset-bot bot commented Sep 15, 2023

🦋 Changeset detected

Latest commit: ad1ca49

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
ember-scoped-css Minor
ember-scoped-css-compat Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR


const COMPONENT_EXTENSIONS = ['hbs', 'js', 'ts', 'gjs', 'gts'];
const TEMPLATE_EXTENSIONS = ['hbs', 'gjs', 'gts'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will be using these for gjs/gts support later

if (templateOriginal !== templateRewrite) {
// this is an awful hack because we don't know how to invalidate broccoli-persistent-filter cache
// ideally we'd invalidate the broccoli-peristent-filter cache here
await writeFile(templateFilePath, templateContents + ' ');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel sad that I had to revert to using a hack like this, but the cache invalidation was not working as advertised (or I was just doing it wrong). Would love to fix this to actually invalidate the cache correctly, but this will work for now.

@@ -67,12 +71,57 @@ class ScopedFilter extends Filter {
return '';
}
}
async postProcess(results, relativePath) {
if (relativePath.endsWith('.module.css')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should probably be a config option, but I think since usage is low, we can skip it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a config option to opt in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Misunderstood, will look at doing that in a later PR since module.css is also considered elsewhere.

However, I do think I'd like to check for CI to skip the whole thing since CI doesn't do a second pass and we'd be doing extra work for nothing.

   if (process.env.CI || relativePath.endsWith('.module.css')) {
      return results;
    }

@jakebixby
Copy link
Contributor Author

Trying to figure out how to test this specific functionality, it's very dependent upon changing files, which isn't really in the scope of an integration or even acceptance test. Any advice would be welcome!

);
} else {
// find all template tags, and extract the contents to compare
const templates = parseTemplates(templateRaw, '');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

parses templates with ember-template-tag, which returns an array of identified template tag objects, with contents representing the template markup. We can then use this for the template change check and if any did, we append a space to the file to kick the cache.

@NullVoxPopuli NullVoxPopuli merged commit 728e4bd into main Sep 18, 2023
8 checks passed
@NullVoxPopuli NullVoxPopuli deleted the template-rebuild-on-css-postprocess branch September 18, 2023 20:02
@github-actions github-actions bot mentioned this pull request Sep 18, 2023
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