UI - core addon & replication engine #6629
Conversation
|
|
||
| export default { | ||
| initialize, | ||
| }; |
There was a problem hiding this comment.
This initializer is key to the strategy we're going to adopt going forward. We'll be able to do logic / fetch info from the cluster, and then optionally enable engines we've abstracted from the current application. This will let us offer a more customized and streamlined experience for users (possibly even mixing up the route structure in the future).
|
|
||
| const environment = EmberApp.env(); | ||
| const isProd = environment === 'production'; | ||
| const isTest = environment === 'test'; | ||
|
|
||
| module.exports = function(defaults) { | ||
| var app = new EmberApp(defaults, { | ||
| assetLoader: { |
There was a problem hiding this comment.
This was needed to get the engine assets working with our rootURL config.
| "ember-truth-helpers": "*", | ||
| "ember-concurrency": "*", | ||
| "base64-js": "*", | ||
| "Duration.js": "*" |
There was a problem hiding this comment.
This is one of the nice things about in-repo-addons IMO - package version management is done in the host application's package.json, you add deps here to help the build step, but there's no need to specify versions separately.
meirish
changed the title
|
Tests won't run here since the PR's not against master, but ^ is from a local test run. |
|
|
||
| setApplication(Application.create(config.APP)); | ||
| preloadAssets(manifest).then(() => { | ||
| setApplication(Application.create(config.APP)); |
There was a problem hiding this comment.
Just to be sure, will this load the dependencies for all additional new engines as well?
There was a problem hiding this comment.
Yeah, any engines with lazyLoading: true configured. Here's the docs for it: https://github.com/ember-engines/ember-asset-loader#pre-loading-assets-during-testing
| </progress> | ||
| {{/if}} | ||
| {{else}} | ||
| <ICon @glyph="neutral-circled-outline" @size="16" @class="has-text-grey-light" /> |
There was a problem hiding this comment.
should this instance of ICon be commented out as well?
There was a problem hiding this comment.
Heh, probably - just a branch that's hard to hit locally.
| <p class="has-text-centered"> | ||
| {{#upgrade-link linkClass="button is-ghost has-icon-right" pageName="Performance Replication"}} | ||
| Learn more | ||
| {{i-con glyph="chevron-right"}} |
| {{#if replicationEnabled}} | ||
| {{#if (get cluster (concat mode 'StateGlyph'))}} | ||
| <span class="has-text-success"> | ||
| {{i-con size=16 glyph=(get cluster (concat mode 'StateGlyph'))}} |
| <div class="level-left is-flex-1"> | ||
| {{#if replicationUnsupported}} | ||
| Unsupported | ||
| {{else if replicationEnabled}} |
There was a problem hiding this comment.
this template has a lot of conditionals in it and it can be hard to keep track of what's going on. could it be split up into smaller components?
There was a problem hiding this comment.
Yeah, before it was a couple of partials so it was easier to follow. I spent a while trying to get components to work with the larger one and couldn't get it right when using components so this (inlining the templates) was my fall back :-/ - I think pretty much this whole engine needs a rewrite. This one may be easier to divvy up than the other one I was looking at though, I'll give it a try.
There was a problem hiding this comment.
Gotcha. I think it's worth trying, but I don't see it as a blocker. Just makes things easier to read. 😬
| @@ -42,7 +42,7 @@ | |||
| </div> | |||
| </div> | |||
| {{#each mounts as |mount|}} | |||
| {{#unless (or mount.local (includes singletonMountTypes mount.type))}} | |||
| {{#unless (or mount.local (contains mount.type singletonMountTypes))}} | |||
There was a problem hiding this comment.
I thought contains only worked in firefox? Does this work?
There was a problem hiding this comment.
This is contains from the ember-composable-helpers addon - https://github.com/DockYard/ember-composable-helpers#contains - includes here was a custom helper we wrote, so I've deleted that in favor of contains from the addon we were already using.
| }} | ||
| {{/if}} | ||
| </div> | ||
| {{else}} |
There was a problem hiding this comment.
😳 oh golly, this template is also quite large. is it possible to break it up?
There was a problem hiding this comment.
😬 this is the one I tried to break up, but it wasn't super straightforward. I'm gonna try doing the summary mode one, but think this one might be hard to break up without a larger refactor.
| "ember-addon", | ||
| "ember-engine" | ||
| ], | ||
| "dependencies": { |
There was a problem hiding this comment.
ah okay, so we need to add all engine-specific dependencies to each engine's package.json as well as the core/package.json?
There was a problem hiding this comment.
Oh nice catch! We might be able to get away with removing these two because we rely on core which relies on them. They were part of the default blueprint when generating an in-repo-engine and you'd normally need them out of the box for template and cross-compilation concerns. I'll try removing them and see if that works.
There was a problem hiding this comment.
Aha so it looks like these are required because of template compilation. Removing them I got these errors and a failed build:
DEPRECATION: Addon files were detected in `/Users/meirish/go/src/github.com/hashicorp/vault/ui/lib/replication/addon`, but no JavaScript preprocessors were found for `replication`. Please make sure to add a preprocessor (most likely `ember-cli-babel`) to in `dependencies` (NOT `devDependencies`) in `replication`'s `package.json`.
Addon templates were detected, but there are no template compilers registered for `replication`. Please make sure your template precompiler (commonly `ember-cli-htmlbars`) is listed in `dependencies` (NOT `devDependencies`) in `replication`'s `package.json`.
|
Holy 🐮! Matthew, this is so awesome. I can see it was a lot of work and I'm really excited to see the payoffs. I really appreciate how much effort you put in the PR description and comments -- it made the code review much easier, and it'll be tremendously helpful if we ever need to come back to this PR as an example. I had a few questions about splitting things up and other small, non-blocking comments. The one meaty thing I want to bring up though is Storybook. Since this PR moves several components and partials into a shared library, does it make sense to add any of them to Storybook? I realize this adds a bit more work to an already large PR, so I'm open to your ideas. It'd be nice to get at least one or two additional components into Storybook; otherwise I think we risk putting it off too far into the future where our Storybook becomes stale. 😞 |
…ion engine, remove some partials
meirish
force-pushed
the
ui-component-addon
branch
from
f00b9ad
to
1b7c5fd
Compare
…ror, popup-menu, ttl-picker, and vault-logo-spinner to storybook
|
OK! Thanks for reviewing this - I think I'm going to merge this now, but can still address additional feedback in a follow up PR, so don't hesitate to add more comments! I added a bunch of the core components to storybook after fixing an error parsing out the manifest in storybook (storybookjs/ember-cli-storybook#9) - that still won't enable using lazy engines in storybook, but that's not something we need at the moment and can look at supporting when we get to it. I left the ICon's alone rather than commenting since the follow on work that I've already started it just replacing all of those with a new component. For the bigger templates, I created a tech-debt ticket to come back and follow up on them. I think it'd be worth looking at adding more routes so that so many different states aren't represented at the same urls. |
This is the first of a few PRs to a feature branch to start to split our ember application into a few pieces. We'll be doing this by moving shared code into an addon, and code we want to lazily enable / isolate into Ember Engines - engines are essentially mini Ember applications that can be mounted and enabled in a host application (or another engine) - more can be read about them here: http:https://ember-engines.com/guide/what-are-engines.
In this PR, we've created an in-repo add-on called
coreto house the code that the host application shares with engines, and an in-repo-engine calledreplicationthat includes all of the replication management routes, controllers, components, etc. Both of these being in-repo was a conscious decision because it simplifies dependency management and developer workflow while still providing the benefit of isolation and (for now) lazy loading of the engine code. The host app'spackage.jsonmanages all dependencies, and all ember-cli commands are still run from theui/directory, not the individual addons.The motivation for the split are twofold. First, engines are currently the only stable way to lazy load application code, so splitting out the replication code means all users will get a lighter initial payload, and for OSS users, they'll likely never load the package at all. Second, we want to explore new UI ideas that may remix the existing UI or even load entirely different interfaces depending on a either a user configuration or a server configuration - the bulk of the shared code for that is likely in components so a shared addon is how we'd do that and any new routes / UIs could be built as engines and conditionally enabled in the
enable-enginesinitializer.New directories
lib/coreThis is the in-repo addon for code that is shared between the host app and any engines. Both the host app and the engine rely on it as a dependency. Components and helpers that were used in both places were moved here, mostly that looked like this:
mv app/components/name.js lib/core/addon/components/.,mv app/templates/components/name.hbs lib/core/addon/templates/components/.,ember g component-addon name -ir core. The first moved the JS file, the second the template, and the third adds a file atlib/core/app/components/name.jsthat re-exports the addon component. Additionally, the JS file needed to be edited to import the template, and then inline it aslayoutin the component definition.lib/replicationThis is the in-repo engine, it now gets mounted at runtime by an initializer. Currently we're not doing any logic in that initializer, but in the future we may conditionally enable parts of the application to allow more flexible configuration of the UI. Routes, controllers, templates, components, etc. went here only if they were solely used in the replication routes. Moving items into the engine only requires the first two steps:
mv app/components/name.js lib/replication/addon/components/.,mv app/templates/components/name.hbs lib/replication/addon/templates/components/.. This is because the Engine base class does some additional things to re-export automatically at build time.Dependency management
Packages and addons
Packages that are required in either the addon or the engine are added to
core'spackage.jsonunder thedependencieskey. Since it's an in-repo addon, we don't need to specify a specific version, but in order for packages to end up available to the right parts of the app, they do need to be listed here. https://github.com/hashicorp/vault/pull/6629/files#diff-6e9c331e643ef4f090faa7146d62826dR1 - packages listed here are available to code in the addon and code in any engine that depends on the addon.Services
Any services that are shared between the host app and an engine must be declared on both sides in the app's and engine's config. See https://github.com/hashicorp/vault/pull/6629/files#diff-a12b9572e58538cd46d06e71502443c3R16 and https://github.com/hashicorp/vault/pull/6629/files#diff-fbf1716654ac42962d210ce002291615R11
Testing
There were a couple of things that are worth pointing out regarding testing.
Since the replication engine is lazy loaded, we needed to ensure all of its deps are loaded before starting up the tests, that is done here: https://github.com/hashicorp/vault/pull/6629/files#diff-a7de7e38c35f234d623a85315a97248cR9.
Testing components that lived in the replication engine requires you to set the test resolver to the engine's resolver since each engine has its own namespace and container. We do that here: https://github.com/hashicorp/vault/pull/6629/files#diff-f443f0e80adc54f245854a0014683092R35
Other notes
Partials don't get loaded properly when shared across addons/engines, so this PR moves many partials to components which is eventually necessary as partials will be deprecated in a future Ember release.
Future PRs to this feature branch: