-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
ui: Runtime Injectable Components #11969
Conversation
2f7138c
to
dbc38ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes:
.join(''); | ||
data[`${appNameJS}Routes`] = JSON.stringify(json); | ||
(json, data = (typeof document !== 'undefined' ? document.currentScript.dataset : module.exports)) => { | ||
data[`routes`] = JSON.stringify(json); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is was changed to remove the need for the data-app-name="consul-ui"
attribute from the HTML <script>
tags that we use for 'importing' these configuration files. As mentioned in the PR description we only used those originally for app namespacing purposes, which is currently overkill for what we need so removing it makes these files a little more grokable.
The document/module conditional thing is so we can 'import' whether we are in a browser environment or a NodeJS environment, it works for both.
(not in this PR but for context) The original reason for having this code in a shorthanded anonymous function down the bottom here was to keep this code "out of your face" when you come to this file to edit routing - it's down as the bottom, out of sight, out of mind and also has its own scope so doesn't pollute global.
@@ -62,5 +61,3 @@ as |partition|}} | |||
</li> | |||
{{/if}} | |||
{{/let}} | |||
{{/if}} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need to use these conditionals around all our components that are influenced by a feature flag as we inject different implementations of the component from The Outside depending on the feature flag. All in all this is "1 conditional of out sight out of mind vs potentially bazillions of conditionals all over the codebase".
@@ -0,0 +1,9 @@ | |||
(services => services({ | |||
"component:consul/partition/selector": { | |||
"class": "consul-ui/components/consul/partition/selector" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use this class for the Consul::Partition::Selector
component only if this config file has been 'imported' (which is only when Consul has set the PartitionsEnabled
feature flag to true
)
JSON.parse($item.dataset[`${appNameJS}Services`]) | ||
) | ||
[...doc.querySelectorAll(`script[data-services]`)].map($item => | ||
JSON.parse($item.dataset[`services`]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More simplified <script src="the-config-file.js" />
slurping/'importing'.
} | ||
}); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR: Yey prettier
. Nothing to see here, please move on, do not pass go, do not collect $200 😄
Previous to this PR: This is where we do:
// container here is pretty much the same as the ember `application`
container.register('component:consul/partition/selector', 'consul-ui/components/consul/partition/selector');
Additionally here, we do an extra replacement to remove the word torii
from our application config (we might switch that out at some point not to use the torii
addon)
{{end}} | ||
{{if .NamespacesEnabled}} | ||
<script data-app-name="${appName}" data-${appName}-routing src="${rootURL}assets/consul-nspaces/routes.js"></script> | ||
<script src="${rootURL}assets/consul-nspaces/routes.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where we 'import' or don't 'import' additional configuration depending on what Consul says, things like .NamespacesEnabled
are set to true/false directly by the backend (our index.html file is run through go-template before it gets served up to the user). Note: currently we only have injectable service/component for partitions currently - once this PR is merged we will be using the same approach for Namespaces and ACLs and more.
@@ -72,7 +74,8 @@ ${ | |||
if(get(key) || (key === 'CONSUL_NSPACES_ENABLE' && ${ | |||
env('CONSUL_NSPACES_ENABLED') === '1' ? `true` : `false` | |||
})) { | |||
document.write(\`\\x3Cscript data-app-name="${appName}" data-${appName}-routing src="${rootURL}assets/\${value}/routes.js">\\x3C/script>\`); | |||
document.write(\`\\x3Cscript src="${rootURL}assets/\${value}/services.js">\\x3C/script>\`); | |||
document.write(\`\\x3Cscript src="${rootURL}assets/\${value}/routes.js">\\x3C/script>\`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only to let us do the same feature flagging during development (when we don't have Consul telling us what is enabled). Here the feature flagging is based on the engineers cookies. Note: If this wasn't development I'd be avoiding the document.write
.
json, | ||
data = typeof document !== 'undefined' ? document.currentScript.dataset : module.exports | ||
) => { | ||
data[`routes`] = JSON.stringify(json); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This route is only complied/enabled if you are in development mode (note the routes-debug.js name of the file). It's the page/route for our fake OIDC provider we use for manually testing SSO (#11248)
json, | ||
data = typeof document !== 'undefined' ? document.currentScript.dataset : module.exports | ||
) => { | ||
data[`services`] = JSON.stringify(json); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Development time only routes for:
- application: This adds our docs menu/navigation if you go to anything under
/docs
- intl: various development utilities to help us convert the entire app to 100% support i18n (eventually! 😄 )
}, | ||
'component:consul/partition/selector': { | ||
class: '@glimmer/component', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default (if partitions aren't enabled) the Consul::Partition::Selector
is just an empty component (an direct instance of @glimmer/component
)
There are also some other injections we need here, but all of this file is always loaded i.e. these are production injections.
@@ -27,6 +27,7 @@ module.exports = function(defaults, $ = process.env) { | |||
let excludeFiles = []; | |||
|
|||
const apps = [ | |||
'consul-ui', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We added consul-ui
itself as an 'external application' here even though its currently the 'host application' - this is important to help with a comment further down (https://github.com/hashicorp/consul/pull/11969/files#r783064423)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code 👍
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/557938. |
This PR does several things that are all slightly related:
1. Configuration files
When we first added these I tried made them uber non-polluting, so everything was using the
appName
to ensure we only used the configuration files from our app. If this was a bigger 'thing' then this would be important to do, but I felt in the scheme of things it was more important to make these files as simple and grokable as possible. So I took out all theappName
things. We control the entire app and distribution so the non-pollution aspect isn't super important. If it ever becomes important we can always put this stuff back.2. Explicit configuration rather than convention
These configuration files are now consumable both in a browser and in a CommonJS environment (e.g. NodeJS/ember-cli). This means we can easily reuse this configuration both in the browser environment during runtime and in the ember-cli/node environment during build time. This means we begin to know the exact filenames that we need to exclude from a production build instead of relying on wildcarding/globbing and naming conventions such as
*-debug
.Note: I haven't removed the previous wildcarding/convention based approach as yet as we still need to make sure that things other than component and services can be excluded in this way (for example templates).
3. Injectable components at runtime
We have various features such as ACLs, Namespaces and Partitions which are optionally activated depending on what version of Consul you are running or what configuration you have (or potentially in the future, what other apps/services you have running). Currently features/components related to these features are added to the UI but enabled/disabled via various conditionals (using
ember-can
) dotted all over the code base.Ideally, instead of having these components all over the app with conditionals wrapped around them, we would enable/disable them in one place in configuration at runtime. This PR is the first instance of us doing this.
Here for example, when running the app without partitions enabled, the
Consul::Partition::Selector
component is simply an instance of@glimmer/component
i.e. an empty component that does absolutely nothing - kind of like a noop. Whereas when partitions are enabled, Consul itself (from the backend) therefore sources/loads the partition service configuration file which injects a different implementation ofConsul::Partition::Selector
(consul-partitions/components/consul/partition/selector
) and the component then 'comes alive'. All in all this prevents us from having to wrap components in conditionals all over the codebase, we just add them where they are meant to be when they are 'brought to life'.I've only done one component here mainly for example purposes, but there will be a follow up PR removing all the top-level conditionals from these app components (all those in
ui/packages/consul-{partitions,acls,nspaces}
.All that is left to do now here towards this endgame (being able to load completely separate applications into our app at runtime) is separating off the source code for the external applications into separate JS files and sourcing those separately to the main
consul-ui.js
source file. Once we finish that up we can potentially ship features for these external applications on a completely different release schedule to the main Consul binary.