-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
Revision compiler revised #2805
Conversation
I love this! :) |
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.
Changes look good to me, including the differentiator changes. Local testing confirms that less is recompiled when changed as expected. Just a slight nitpick, commit
assumes debug mode (because it's only called during debug mode). Can we explicitly state this expectation either in a comment, or by renaming to something? forceCommit
maybe?
If the method is named commit, it feels superfluous to name the variable forceCommit. That's a waste of screen width and only makes it unclear as I'd expect something else to be forceable too 🙈 |
I meant renaming the method in the |
Changes proposed in this pull request:
Moving from
<asset>-<revision>.<js|css>
to<asset>.<js|css>?v=<revision>
. As a consequence we no longer need to delete the old file, browsers will automatically pick up that a new file was generated.Additional changes
time()
causing a recompile every time assets are compiled; but as all sources (string, file) offer their own differentiator and the less compiler only adds import dirs, I've used that as another differentiator. Let me clarify the impact:For production sites there won't be any change, except that css is compiled less often.
For development environments where import dirs see changes this might be problematic, however this PR also takes this into consideration by allowing the
commit()
method of the recompiler to get an argument$force (false)
to force recompilation when in debug mode.Reviewers should focus on:
Whether we want to modify the differentator for Less like this.
Screenshot
Confirmed
composer test
).Required changes: