bootstrap ui in script file rather than inline#15904
Conversation
epixa
added
Team:Core
epixa
force-pushed
the
remove-inline-scripts
branch
from
38239fc to
2ec48a3
Compare
spalger
left a comment
spalger
left a comment
There was a problem hiding this comment.
Just some notes, excited about the direction here
There was a problem hiding this comment.
Just curious, why not just hash the result of #getJsFile()? Feels like that is what this method is claiming to do, but is avoiding it for some reason.
There was a problem hiding this comment.
Good idea. I arrived at this current implementation after some refactoring, and it's not ideal.
src/ui/ui_render/ui_render_mixin.js
Outdated
There was a problem hiding this comment.
If we put everything within the try {} catch {}, and threw the error on 41 rather than passing it directly to reply(), then this handler would have a single flow for errors and would properly route errors if server.getUiAppById() threw, or request.params wasn't defined for some reason.
src/dev/jest/config.json
Outdated
There was a problem hiding this comment.
This could probably just replace the next line
There was a problem hiding this comment.
Why is the translation for the key foo in the js file?
There was a problem hiding this comment.
The key is foo in the mock template for this test. In the actual template, it is UI-WELCOME_ERROR, which is the translation key for the error message that is shown when bundles fail to load properly: https://github.com/elastic/kibana/blob/master/src/ui/ui_render/views/ui_app.jade#L143
|
@spalger I pushed changes for your feedback |
|
This is breaking for the status page. I'll check it out tomorrow. |
|
#15880 was merged, so this PR is unblocked |
There's no good reason to bootstrap the UI within the browser using an inline script tag rather than a separate dynamically created bootstrap script. Having this bootstrapping logic in its own file allows us to cache this separately from the main HTML payload, and it is required if we want to disable inline scripts entirely.
epixa
force-pushed
the
remove-inline-scripts
branch
from
cb020d8 to
d271851
Compare
💚 Build Succeeded |
|
@spalger Sorted out the status page issue and ready for another look. |
|
|
||
| server.route({ | ||
| path: '/bundles/app/{id}/bootstrap.js', | ||
| method: 'GET', |
There was a problem hiding this comment.
Make sure this gets config: { auth: false }
There was a problem hiding this comment.
Good call. I pushed this up and merged with the latest master.
We do not require auth for the rest of our static JS files, and this file should not be treated differently than those. That this file is created dynamically rather than pulled directly from the file system is just an implementation detail.
💔 Build Failed |
|
jenkins, test this |
💚 Build Succeeded |
|
I pinged the visualizations team about that random test failure. It was some threshold-based test failing in a flaky way, so they're looking into it. |
* bootstrap ui in script file rather than inline There's no good reason to bootstrap the UI within the browser using an inline script tag rather than a separate dynamically created bootstrap script. Having this bootstrapping logic in its own file allows us to cache this separately from the main HTML payload, and it is required if we want to disable inline scripts entirely. * cleanup from review 1 * fix status page bootstrap * do not require auth for app bootstrap files We do not require auth for the rest of our static JS files, and this file should not be treated differently than those. That this file is created dynamically rather than pulled directly from the file system is just an implementation detail.
* bootstrap ui in script file rather than inline There's no good reason to bootstrap the UI within the browser using an inline script tag rather than a separate dynamically created bootstrap script. Having this bootstrapping logic in its own file allows us to cache this separately from the main HTML payload, and it is required if we want to disable inline scripts entirely. * cleanup from review 1 * fix status page bootstrap * do not require auth for app bootstrap files We do not require auth for the rest of our static JS files, and this file should not be treated differently than those. That this file is created dynamically rather than pulled directly from the file system is just an implementation detail.
3 participants
There's no good reason to bootstrap the UI within the browser using an
inline script tag rather than a separate dynamically created bootstrap
script.
Having this bootstrapping logic in its own file allows us to cache this
separately from the main HTML payload, and it is required if we want to
disable inline scripts entirely.
The UI bootstrapping logic is now served up on an app-specific .js
endpoint. The URL for the boostrap must contain the app id so that it
can load the relevant app-specific JS bundles on the fly.
The HTTP caching will be invalidated whenever the underlying template
changes, the interpolated data changes, or any translations change.
This is necessary for #2873