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

Cache bust #1350

Closed
wants to merge 26 commits into from
Closed

Cache bust #1350

wants to merge 26 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 22, 2017

Template middleware return a jade compile function
minified or not with unique hash by template, the client side
get a template hash from a list where live availables templates,
aditional set a max-age expiration for better render performance
in client-side and is more friendly with development because not
need build templates manually.

Closes: #1204

@ghost ghost mentioned this pull request Apr 25, 2017
Copy link
Member

@strugee strugee left a comment

Choose a reason for hiding this comment

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

Hey, so sorry - this dropped off my radar somehow and I forgot about it :(

I started going through and leaving comments but stopped since I'm not sure this is the best approach - AFAICT this compiles templates each startup and caches them in memory, right? I'm not sure this is better than having a build step that writes templates back to disk, similar to what we currently have. That makes startup faster and it improves memory usage.

I'm not really sure, honestly.

lib/template.js Outdated
//
// Template middleware with cache-buster
//
// Copyright 2012, E14N https://e14n.com/
Copy link
Member

Choose a reason for hiding this comment

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

This should be adjusted :)

Copy link
Author

Choose a reason for hiding this comment

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

made

lib/template.js Outdated
var fs = require("fs"),
path = require("path"),
crypto = require("crypto"),
_ = require("underscore"),
Copy link
Member

Choose a reason for hiding this comment

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

Since this was submitted we've migrated to Lodash so this needs to be adjusted. Sorry, that's my bad :(

Copy link
Author

Choose a reason for hiding this comment

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

made

lib/template.js Outdated
"// \n" +
"// @licend The above is the entire license notice\n" +
"// for the JavaScript code in this page.\n\n" +
"// XXX: this needs to be broken up into 3-4 smaller modules \n";
Copy link
Member

Choose a reason for hiding this comment

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

This probably doesn't belong here?

lib/template.js Outdated
"// @licstart The following is the entire license notice for the \n" +
"// JavaScript code in this page.\n" +
"// \n" +
"// Copyright 2011-2013, E14N https://e14n.com/\n" +
Copy link
Member

Choose a reason for hiding this comment

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

This should probably read:

Copyright 2011-2017, E14N https://e14n.com/ and contributors

Copy link
Author

Choose a reason for hiding this comment

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

made

lib/template.js Outdated
"// XXX: this needs to be broken up into 3-4 smaller modules \n";


// Get al files from views directory but only take all .jade files
Copy link
Member

Choose a reason for hiding this comment

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

all instead of al

Copy link
Author

Choose a reason for hiding this comment

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

made

lib/template.js Outdated
templateContent = fs.readFileSync(templatePath, {encoding: "utf8"}),
// Compile jade template for client side
templateFn = jade.compileClient(templateContent, {
externalRuntime: false,
Copy link
Member

Choose a reason for hiding this comment

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

What is this option? I don't see it in the docs; maybe you meant inlineRuntimeFunctions?

Copy link
Author

Choose a reason for hiding this comment

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

In jade 1.11.x both optionsexternalRuntime or inlineRuntimeFunctions not exist, see: https://github.com/pugjs/pug/tree/master/packages/pug-runtime pugjs/pug@1eeb9e6

@ghost
Copy link
Author

ghost commented Sep 2, 2017

Memory is always faster than read from disk (example swap vs zswap), reduce IO-Operations is always better for performance, this option is also more friendly for frontend developers and don't increment much the startup time only increment seconds more

https://www.backblaze.com/blog/whats-diff-ram-vs-storage/
https://stackoverflow.com/questions/25467993/node-js-performance-reading-files-once-or-on-every-request

@coveralls
Copy link

coveralls commented Oct 9, 2017

Coverage Status

Changes Unknown when pulling 0cdf6ec on vxcamiloxv:cache-bust into ** on pump-io:master**.

@coveralls
Copy link

coveralls commented Oct 9, 2017

Coverage Status

Coverage increased (+0.2%) to 73.974% when pulling 572780a on vxcamiloxv:cache-bust into 8fb1962 on pump-io:master.

@coveralls
Copy link

coveralls commented Oct 9, 2017

Coverage Status

Coverage increased (+0.2%) to 73.974% when pulling 572780a on vxcamiloxv:cache-bust into 8fb1962 on pump-io:master.

@coveralls
Copy link

coveralls commented Oct 16, 2017

Coverage Status

Coverage decreased (-3.6%) to 70.187% when pulling c7c3b8b on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-3.6%) to 70.187% when pulling c7c3b8b on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.6%) to 70.187% when pulling c7c3b8b on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

@coveralls
Copy link

coveralls commented Oct 16, 2017

Coverage Status

Coverage decreased (-3.7%) to 70.149% when pulling 9c7412f on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 73.96% when pulling 9c7412f on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 73.96% when pulling 9c7412f on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

@coveralls
Copy link

coveralls commented Oct 16, 2017

Coverage Status

Coverage increased (+0.2%) to 73.96% when pulling 9c7412f on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 73.96% when pulling 7f76f89 on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 73.96% when pulling 7f76f89 on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

@coveralls
Copy link

coveralls commented Oct 16, 2017

Coverage Status

Coverage increased (+0.2%) to 73.96% when pulling 7f76f89 on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

@coveralls
Copy link

coveralls commented Oct 17, 2017

Coverage Status

Coverage increased (+0.2%) to 73.96% when pulling d07e89e on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

Camilo QS added 12 commits October 17, 2017 13:23
This middleware returns a jade template compile function
minified or not with unique hash by template, the client side
get a template hash from a list where live availables templates,
aditional set a max-age expiration for better render performance
in client-side and is more friendly with development because not
need build templates manually.

Related: #1204
'inlineRuntimeFunctions' option not exist in jade 1.x,
Added for compatibility in future 'pug' upgrades
@coveralls
Copy link

coveralls commented Oct 17, 2017

Coverage Status

Coverage increased (+0.2%) to 73.975% when pulling 59ee061 on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

@coveralls
Copy link

coveralls commented Oct 17, 2017

Coverage Status

Coverage increased (+0.2%) to 73.975% when pulling 59ee061 on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

@ghost
Copy link
Author

ghost commented Oct 18, 2017

@strugee I was thinking about other ways to make better this implementation and if you still thinking keep in memory could be have performance issues I have two options.

Server side options

A) The template (should be rename to cachebuster) middleware currently read a template from disk and serve this to client in debug mode, I could be will keep this behavior in production but with pre-compile and minified templates (and other statics)

B) I'll modified the request path and remove the hash, Will delegate the response to express-statics middleware.

Client side options.

A) Reads all statics files and make hash, keeps in memory (only name file and hash)

  • For template, a templates.js with files names and hash for every file or append to Pump._templates in layout.jade as JSON
  • For other statics reads all from disk and generate hash, keeps in memory.

B) A build task (a script in package.json) for generate a JS file and requires as node dependency, the file could be have this structure:

exports {
    js: [
        {"file-name": "hash"}
    ],
    css: [
        {"file-name": "hash"}
    ],
    template: [
        {"file-name": "hash"}
    ]
}
  • For templates same as option A
  • For other statics generate the file url, like this:
script(src=getFileHash("/javascript/dist/pump.min.js"))

and the results

<script src="/javascript/dist/pump.min.hdg6252.js"></script>

Generation options

A) Generate and minified all statics in for example javascript/dist/ and templates/dist/ by a script task in package.json

B) Generate, minified and saves all statics by server on startup (developer friendly)

What do you think?

@ghost ghost closed this Oct 18, 2017
@ghost ghost reopened this Oct 18, 2017
@coveralls
Copy link

coveralls commented Oct 18, 2017

Coverage Status

Coverage decreased (-3.6%) to 70.187% when pulling c7c3b8b on vxcamiloxv:cache-bust into d1037f2 on pump-io:master.

@ghost
Copy link
Author

ghost commented Oct 24, 2017

I close this PR because has 9 months, and I was thinking in a better and more generic approach for that (not only templates), I Will work over options the previous comment.

@ghost ghost closed this Oct 24, 2017
@ghost ghost mentioned this pull request Mar 15, 2018
This pull request was closed.
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.

None yet

2 participants