Skip to content

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.

/**
* returns max mtime for file paths given in array
*
* @param filePaths array of file paths
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.

$js = $js . "\n" . $this->minifyJs(file_get_contents(\F3::get('BASEDIR') . '/' . $file));
private function genMinified($type) {
self::$staticmtime[$type] = self::maxmtime(\F3::get($type));

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.

*
* @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.

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

helpers/View.php Outdated
$css = '';
foreach (\F3::get('css') as $file) {
$css = $css . "\n" . $this->minifyCss(file_get_contents(\F3::get('BASEDIR') . '/' . $file));
// cleanup
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 force-pushed the ppr/static-gen-mtime branch from 71503d4 to 4ac4000 Compare April 4, 2017 14:45
@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