Adjustments to livereload to stop (timeout) in prod #216
Conversation
|
@binarymist due to the code style changes (prettier) it's hard to find what are the actual updates here. |
|
Hi @lirantal Configuration The livereload script tags scattered through the views have been moved into one place... now living in an array of Routes The routes simply pass the Views Views and their owning layouts now contain a place holder for any environmental scripts.
Thanks. |
|
How are we looking @lirantal, any other thoughts? |
|
Okie, happy to RSLGTM it - just a note in the future it would be easier and quicker to review with just the functional changes and code styles left out of it. |
|
@binarymist thanks for the PR. It is great that the PR solves challenges you and rob are facing. |
config/env/all.js
Outdated
| @@ -13,5 +13,6 @@ module.exports = { | |||
| cookieSecret: "session_cookie_secret_key_here", | |||
| cryptoKey: "a_secure_key_for_crypto_here", | |||
| cryptoAlgo: "aes256", | |||
| hostName: "localhost" | |||
| hostName: "localhost", | |||
| environmentalScripts: ["<script src='//localhost:35729/livereload.js'></script>"] | |||
There was a problem hiding this comment.
Previously the host name in script tag was derived as location.host || "localhost". I believe this was done to support development on cloud based IDEs where hostname is not localhost. Do you see any issues if we retain the same logic, instead of hardcoding it to localhost?
There was a problem hiding this comment.
Any suggestions on how to defer the evaluation of location until it hits the browser?
There was a problem hiding this comment.
I guess we could have the environmentalScripts as an array that looks something like the following:
[{
scriptTag: '<script src="//\`{{script.substitutions.loc}}\`:35729/livereload.js"></script>',
substitutions: { loc: '${location.host || "localhost"}' } // No backticks so no eval.
}, {
// Future scripts
}]I'm not quite sure on the best way to get the environmentalScripts into the view yet, would need to play and see what's possible with the current templating lib. Ultimatly the script.substitutions.loc needs to slot into the scriptTag.
It's quite a bit of extra work, but what ever needs to be done.
Plausable @ckarande ?
There was a problem hiding this comment.
We could use the same trick as the original livereload script block, ie make the environmentalScripts line:
environmentalScripts: [`<script>document.write("<script src='http://" + (location.host || "localhost").split(":")[0] + ":35729/livereload.js'></" + "script>");</script>`]
The document.write happens in the browser, injecting a script tag with the correct livereload.js URL.
|
Just thinking @ckarande and @lirantal, do you think it's worth updating the contributing guide? If sending PR, once code is ready to commit, run:
Please resolve all jsHint errors before committing the code. Based on these two points, I was fairly sure it needed to be done before committing. Updating the contributing guide would help to make sure this doesn't happen again. |
I think this is a good change to make as it stops heroku deploys from including livereload by default. When this change is merged it'll do the same for production docker builds too. Regarding #207, this is a partial fix as it only applies to production environment. As Kim mentioned, we'd also need to remove the livereload script from the test environment to cover Cypress runs in Travis. I still want to update the Cypress config to block requests to port 35729, as I don't think it makes sense to handle live reload during a test run. |
I've been using a docker-composer.override.yml for that, but #215 would be a welcome change. It'd be one less line in my override.
If we simply add the following (taken directly from production.js) to the test.js config then it will resolve #207... yes @rcowsill ? module.exports = {
environmentalScripts: []
};We could of course flip the script and have the livereload script only in the development.js config and have the empty |
That sounds good to me, livereload seems like a development-only feature. |
|
@binarymist LGTM. Thanks for the solution and your patience :) @lirantal @rcowsill Great feedback. As you both are already onboard with the changes, I am merging the PR. |
livereload causes timeout in Zap tests (and cypress by the sound of it) which causes tests to fail.
Most of the noise in this commit is from
npm run precommit, but the important changes means we can add any environmental scripts (script tags) to the environment specific files only (see config/env/all.js for example). In the production environment file I've simply left theenvironmentalScriptsarray empty. Possibly we could do like wise in the test environment file as well, that would fix #207These changes are essential (or at least no livereload) for purpleteam testing NodeGoat.