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

Allow AST plugins' output be cached based on external files #238

Merged
merged 28 commits into from
Jun 28, 2019

Conversation

chriseppstein
Copy link
Contributor

@chriseppstein chriseppstein commented May 28, 2019

In order for certain AST plugins to be cached properly, they may need to include other files into their cache key.

This is useful when you'd like to change the compiled template output based on the contents of a component.css file (which is something that css-blocks would need). Prior to this PR the only way to do this was to have the AST plugin completely opt out of caching (by returning a unique value for its cache key) which obviously has fairly severely negative implications for overall build performance, but is not even enough! Editing the .hbs file would gather any .css file changes, but editing the related .css would still never cause the .hbs file to be recompiled.

addDependencyTracker.js Outdated Show resolved Hide resolved
addDependencyTracker.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
addDependencyTracker.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
node-tests/ast_plugins_test.js Show resolved Hide resolved
node-tests/fixtures/template.tagname Show resolved Hide resolved
@chriseppstein chriseppstein changed the title WIP: Dependency invalidation Dependency invalidation Jun 12, 2019
@chriseppstein
Copy link
Contributor Author

Tests are failling in windows, but passing in os-x. I've setup a dev environment in window and can reproduce the issue now. Will have a fix soon.

@chriseppstein
Copy link
Contributor Author

Tests are green. I had to disable some of the legacy ember version tests for ast plugins because there was no way to unregister plugins once registered.

@chriseppstein
Copy link
Contributor Author

I'd like to squash all these commits down to just 1 commit prior to landing the code.

@chriseppstein chriseppstein marked this pull request as ready for review June 26, 2019 21:53
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Overall looks very good, only one relatively small change needed then we should be good to release.

Thanks for working through this!

addDependencyTracker.js Outdated Show resolved Hide resolved
…emplate node yet, we have to keep a stack counter of Program nodes that we've seen so far.
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your hard work on this!

@rwjblue
Copy link
Member

rwjblue commented Jun 28, 2019

Gonna leave merge + release for the AM (never merge and release just before bed 😝).

@stefanpenner - Lemme know if you have any objections...

@rwjblue rwjblue merged commit d31e851 into ember-cli:master Jun 28, 2019
@chriseppstein
Copy link
Contributor Author

🎉 Thanks @rwjblue & @stefanpenner for helping landing this!

@Turbo87
Copy link
Member

Turbo87 commented Jul 1, 2019

I just saw this PR in the changelog but the title and description do not really describe what it does or why it's an enhancement. Could you elaborate on the purpose of this PR?

@rwjblue rwjblue changed the title Dependency invalidation Allow AST plugins' output be cached based on external files Jul 2, 2019
@rwjblue
Copy link
Member

rwjblue commented Jul 2, 2019

@Turbo87 - Updated title and description to be more descriptive.

@Turbo87
Copy link
Member

Turbo87 commented Jul 3, 2019

thanks 🙏

@chriseppstein chriseppstein mentioned this pull request Aug 12, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants