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

make it unlikely that the client browser gets outdated js or css #907

Merged
merged 1 commit into from
Apr 4, 2017

Conversation

niol
Copy link
Collaborator

@niol niol commented Mar 26, 2017

This change makes the js and css generated to a file containing the
max mtime of source material in its name. Therefore, each time the
source js or css files are changed, a new file with a new name gets
generated and referenced in the app. From the client browser's view,
this is a new ressource that needs to get fetched regardless of the
cache status of older css or js.

The trade-off is that each request implies stat'ing each source
material file. There is no notiecable time penalty in loading time.

helpers/View.php Outdated
* returns max mtime for file paths given in array
*
* @param filePaths array of file paths
* @returns max mtime as int (unix timestamp)
Copy link
Member

Choose a reason for hiding this comment

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

Should be @return int annotation.

helpers/View.php Outdated
private function genMinified($type) {
self::$staticmtime[$type] = self::maxmtime(\F3::get($type));

if ($type == 'js') {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than 'js' and 'css', class constants should be created to decrease the chance of accidental mistypes. Since older versions of PHP do no support private constants, @internal annotations should be used for now.

helpers/View.php Outdated
* @param filePaths array of file paths
* @returns max mtime as int (unix timestamp)
*/
public static function maxmtime($filePaths) {
Copy link
Member

Choose a reason for hiding this comment

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

array type hint should be added since it is supported since PHP 5.1: https://3v4l.org/U4Gq4

helpers/View.php Outdated
foreach (\F3::get('css') as $file) {
$css = $css . "\n" . $this->minifyCss(file_get_contents(\F3::get('BASEDIR') . '/' . $file));
// cleanup
if ($builtSomething) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just use cache busting using query string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One disadvantage of cache busting using a query string is that it does not return a 404 for outdated content. But is saves the cleanup. Anyway, both may lower user support needs.

@jtojnar
Copy link
Member

jtojnar commented Mar 27, 2017

Website and .gitignore should be updated as well.

This change makes the js and css generated to a file containing the
max mtime of source material in its name. Therefore, each time the
source js or css files are changed, a new file with a new name gets
generated and referenced in the app. From the client browser's view,
this is a new ressource that needs to get fetched regardless of the
cache status of older css or js.

The trade-off is that each request implies stat'ing each source
material file. There is no notiecable time penalty in loading time.
@jtojnar jtojnar merged commit 4ac4000 into fossar:master Apr 4, 2017
jtojnar added a commit that referenced this pull request May 6, 2017
Pull request #907 switched to using query string as a cache busting
strategy but the runner for the PHP built-in server did not anticipate
any text after file name extension.

See also: #910
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