Update UI Config passing to not use an inline script #8645
Conversation
|
@johncowen I rebuilt the assetfs by running |
|
LGTM @banks, I checked everything during dev is all ok (we are still able to change these settings via Web Inspector), but I'll leave approval for go folks Thanks for doing this, looks much cleaner from the frontend side. |
| // writing out the new version into a buffer and then serving file reads from | ||
| // that. It assumes you are modifying a real file and presents the actual file's | ||
| // info when queried. | ||
| type bufferedFile struct { |
There was a problem hiding this comment.
http.go is quite large and seems to be mostly the "framework" we use for the http API. It might be nice to move this type to a separate file at some point.
| // We want to remove the first and last chars which will be the { and } since | ||
| // we are injecting these variabled into the middle of an existing object. | ||
| bs = bytes.Trim(bs, "{}") |
There was a problem hiding this comment.
minor: I guess we could probably remove the need for this trim by making the replacement string include the {} and moving the comment around. Possibly even putting it into a separate file, although I'm not sure how easy that is.
There was a problem hiding this comment.
Hmm I don't think so - we are trimming the JSON value that is being inserted here.
The KV pair being matched appears somewhere in the middle of a large JSON blob that contains many other variables that ember sets already.
But I'm not sure what you meant about a separate file so perhaps I'm misunderstanding something?
There was a problem hiding this comment.
Oh ya, I didn't explain this well and thinking through it again my idea changed slightly.
Roughly I was thinking that some other file (module?) could set a CONSUL_INJECTED_UI_SETTINGS (or whatever we want to call it) variable to nil, and environment.js could use that variable as:
ENV = Object.assign({}, ENV, inject.CONSUL_INJECTED_UI_SETTINGS),
where inject is the name of the module that contains the other file (although I don't fully understand how modules work here, so I'm not sure if we can namespace the variable like that).
Then the replacement could replace inject.CONSUL_INJECTED_UI_SETTINGS. The replacement string would not contain any escaped characters (I think), and the source of the variable wouldn't be replaced because it wouldn't include inject..
That would remove the need to trim {} because the content would be an entire JSON object, not just the values, and it would make it much easier to associate these two strings because you could find or grep for them without having to escape things.
There was a problem hiding this comment.
Yeah there is certainly room to make this cleaner but I don't think what you're suggesting works. The environment.js is evaluated at Ember build time. The end result is the index.html that we store in assetFS which has this compiled, encoded JSON object in. We can't inject directly into the runtime JS without doing something more complicated like trying to intercept other JS file requests on the HTTP server and inject at a higher level where the JS has already been minified etc. This seems less bad for now and an improvement over what we were doing before at least!
As this is blocking getting something to work I'd prefer to defer an even larger refactor of how we pass variables to JS - there is already a big TODO in the JS side to change how we pass all of this since it should ideally be in the APP config section and handled in the Application.initialize rather than using env() anyway but that's too much for our frontend engineers to take on in this release cycle.
There was a problem hiding this comment.
If we wanted to inject these things at runtime I think we would probably query them via the API, instead of injecting them into a file, right? I really like that idea, but I assumed there was some reason that wouldn't work. Do we need these settings in order to make API requests?
I think what I was suggesting isn't much different from what you have, but I don't know ember well enough to know if it is possible.
|
@dnephin thanks for catching the typo! Given your 'not blocking' comment and the rationale about a larger refactor above, are you happy to merge this as it is for now or do you think it needs more discussion or another opinion? |
This PR slightly modifies the way we pass through runtime configuration to the UI.
It's a precursor/cleanup before some larger changes to UI config.
The main motivation is to remove the inline script which should allow tighter CSP policies to be used with the UI.
Overall, this is both simpler and slightly more hacky than before however it removed the need for inline script and hopefulyl allows us to easily expand the number of config options we pass to the UI significantly without needing changes in multipele JS files and go files just to plumb template placeholders through.
There is a small Enterprise PR needed once this is merged to rename the method.