Katello - Proposal with working POC - Move bastion to webpack

[John_Mitsch]

Hello all,

We have had some discussions around this and I’ve come up with a working POC so we can discuss further.

Proposal

Move all the angularJS code in Katello into webpack. This includes bastion, our library of modular UI components, and the actual katello pages written in angularJS. For those unfamiliar, this is the majority of Katello’s pages. We have converted a couple (subscriptions and RH repositories) to React and plan to convert more, but this is a large effort that will take a long time. It’s safe to say the angularJS pages will live with us for a long time, though will reduce in number as we convert more to react.

What do we currently do?

We load bastion and the angular pages completely through the Rails pipeline. Since Bastion was meant to be a reusable component, it is a Rails engine.

Benefits

• Having one javascript bundler, webpack, handle the majority of our front-end code.
• Gain any benefits that webpack offers to minify and reduce the amount of code send to the browser.
• Upgrading and optimizing webpack will benefit the angular pages as well.
• Since Katello is the only user of Bastion and that is unlikely to change, remove bastion’s code around making it a reusable rails engine
• Be able to use ES6+ syntax and modern tooling like npm with our angular code
• Simplify js package management to only use NPM (bastion uses bower), more easily upgrade packages
• Could potentially make adding React to Angular pages easier, making our transition to React smoother.

Concerns

• Moving this much code makes cherry-picking for releases more difficult
• May require packaging changes
• While the pages are being moved, duplicate javascript could be sent to the browser since the pages loaded by rails will still use bastion provided by the rails engine and the webpack ones will use bastion provided by webpack. (I’m not 100% sure about this, but worth mentioning)

Strategy

The POC PR moves much of Bastion over to webpack. Ideally, we move Bastion completely over and then work on moving the pages over. In the PR, I’ve moved over the main content credential page, this is to ensure Bastion is working correctly. The page isn’t completely functional yet, but is looking more like the original one.

I think we will want to get Bastion completely working and playing nice with webpack, and then move the top-level pages over one-by-one in separate PRs. This will allow everything to co-exist and not blocked a release or be tied to a time period. The only concern I can think of is duplicating Javascript as I mentioned above.

When a page is moved over, we could do one of two things (afaict):
OPTION 1 - Remove the old page and redirect the top-level page to use the new webpack page. The rails page will no longer be available, and the webpack page will be loaded when the route is used.
OPTION 2 - Move to a namespaced URL, e.g xui/content-credentials. Continue to move all the pages over to this and when they are all fully moved over, remove all old bastion/angular code and switch the routes to load the new webpack pages.

Some other points:

• The business logic of the pages will still be the same. What changes is the importing and exporting the angular components/services/directives/etc. This means if you need to pull in a commit that was done on bastion-in-rails, you should be able to make the same changes easily to bastion-in-webpack.
• We will have to ensure translations still work correctly, as this is a very important part of our app. I plan to handle this in the initial PR.

I would love to hear from everyone, especially @packaging and @ui_ux. Please share any questions, concerns, and/or thoughts on this proposal. Let me know what you think!

[ekohl]

Just some random thoughts.

This may not be an issue, but there are a few plugins that depend on Katello. If they extend the UI, then that’s important to consider as well.

Another thing that the UI team might be more qualified to answer is that you can export things from webpack to be used by the classic rails assets pipeline. Would that be a way of avoiding the need to duplicate? Perhaps the Rails engine could be modified in a way to import from webpack instead of the assets pipeline as a compatibility layer.

Moving this much code makes cherry-picking for releases more difficult

That’s always going to be the case for cherry picks and mayor restructuring. If you’re rewriting a page to react, it’s also going to be hard. We’ll just have to accept that.

OPTION 2 - Move to a namespaced URL, e.g xui/content-credentials . Continue to move all the pages over to this and when they are all fully moved over, remove all old bastion/angular code and switch the routes to load the new webpack pages.

If possible, I’d avoid this.

[John_Mitsch]

Another thing that the UI team might be more qualified to answer is that you can export things from webpack to be used by the classic rails assets pipeline. Would that be a way of avoiding the need to duplicate? Perhaps the Rails engine could be modified in a way to import from webpack instead of the assets pipeline as a compatibility layer.

John_Mitsch:

If this is possible, we can try something like this. I’m currently doing the reverse, using rails assets in webpack, with angular-patternfly in the PR. One thing I could do is measure the actual increase in the size of the JS files sent to the browser, this will give us an idea of the impact and we can make decisions from there.

That’s always going to be the case for cherry picks and mayor restructuring. If you’re rewriting a page to react, it’s also going to be hard. We’ll just have to accept that.

agreed!

OPTION 2 - Move to a namespaced URL, e.g xui/content-credentials . Continue to move all the pages over to this and when they are all fully moved over, remove all old bastion/angular code and switch the routes to load the new webpack pages.

If possible, I’d avoid this.

I agree, the only benefit I would see to this approach is we could completely test all the pages before switching, maybe involving QE. But this would probably be better to do one-at-a-time so we could remove the old code and test individual pages. I lean towards option 1 as well.

[ekohl]

Another thing that the UI team might be more qualified to answer is that you can export things from webpack to be used by the classic rails assets pipeline. Would that be a way of avoiding the need to duplicate? Perhaps the Rails engine could be modified in a way to import from webpack instead of the assets pipeline as a compatibility layer.

If this is possible, we can try something like this. I’m currently doing the reverse, using rails assets in webpack, with angular-patternfly in the PR. One thing I could do is measure the actual increase in the size of the JS files sent to the browser, this will give us an idea of the impact and we can make decisions from there.

In Foreman we have the window.tfm object which is used in the rails assets pipeline, but it’s assigned in webpack:

theforeman/foreman/blob/31c0fb2082cbb86ffdb24c50af2b20b75657150b/webpack/assets/javascripts/bundle.js#L52-L79

Again, the UI team knows more than I do about this, but my impression is that this just works to cross the boundary.

[Walden]

OPTION 1 - Remove the old page and redirect the top-level page to use the new webpack page. The rails page will no longer be available, and the webpack page will be loaded when the route is used.
OPTION 2 - Move to a namespaced URL, e.g xui/content-credentials . Continue to move all the pages over to this and when they are all fully moved over, remove all old bastion/angular code and switch the routes to load the new webpack pages.

I know that option 1 is safer in the event of a breaking bug but we have a history of not going back and removing old pages (the subscriptions page for example). I’d vote for option number 2 if it can be done soon enough in any given release cycle. I think any time we move a page over we should try to do it early in the release cycle.

[Walden]

In Foreman we have the window.tfm object which is used in the rails assets pipeline, but it’s assigned in webpack:

I believe this is something of an anti-pattern. There may be a better way to do this now, perhaps @sharvit or @ohadlevy has an idea?

[sharvit]

In Foreman we have the window.tfm object which is used in the rails assets pipeline, but it’s assigned in webpack:

I believe this is something of an anti-pattern. There may be a better way to do this now, perhaps @sharvit or @ohadlevy has an idea?

I wouldn’t say it’s an anti-pattern since it is the only way (the old way) to communicate with js files lives outside of your building system.
Yes, it’s a bit ugly, and JS used to work this way in the previous generation, and yes, this is one of the reasons developers used to hate javascript and one of the reasons the community developed building tools like babel and webpack.

We do want to remove the window.tfm object eventually but the way to get there is to move the legacy code to webpack and do some rewrites.

So I would say, yes you can use the window object to deliver functions/data to the assets pipline.

1. Use window.katello instead of the window.tfm
2. You should make sure you call window.katello after the document.ready or another event to make sure the window.katello object is ready.

[John_Mitsch]

Another thing that the UI team might be more qualified to answer is that you can export things from webpack to be used by the classic rails assets pipeline. Would that be a way of avoiding the need to duplicate? Perhaps the Rails engine could be modified in a way to import from webpack instead of the assets pipeline as a compatibility layer.

@ekohl Thinking about this, one thing we have to consider is moving the packages is a big part of moving bastion to webpack. We have to make sure they are available in npm, the same version is there, it all imports successfully and is loaded the right way in angular, and everything plays nice with existing code. I’ve already done much of this in the PR. This has actually been the most time-consuming part of the work.

If we still load things from the rails asset pipeline and do not keep them in webpack, we lose a lot of that work and then have to do that later. We’ll have to make sure everything is loaded properly using es6 syntax and works well. IMHO this will make things more complicated and we should make sure the packages loaded with npm and webpack work from the beginning.

I think we need to find a solution where wepback and rails assets can live together. Maybe there is some way we can exclude the npm packages from production until the pages are all ready, or even load them as dev packages. I’ll think some more about it.

I also do want to see the actual impact of the changes in the webpack bundle size so we can make decisions based on some numbers (e.g. a 10% bundle size increase may be something we are willing to live with for a release).

[John_Mitsch]

I know that option 1 is safer in the event of a breaking bug but we have a history of not going back and removing old pages (the subscriptions page for example). I’d vote for option number 2 if it can be done soon enough in any given release cycle. I think any time we move a page over we should try to do it early in the release cycle.

I agree we should do our best to time any large changes to not impact a release. Ideally, this work could be done in between releases and we are switched over completely, with dead code/packages removed.

I’m not sure how long the work will take once the initial changes are merged. I would like to think once bastion is moved over and one page has been moved over as an example, changing the remaining pages would be straightforward and not take long. Of course, this is software, and things don’t always go as expected

Both options would remove the dead code of the pages left behind, and eventually bastion (from the rails side). This is definitely a requirement of this refactor, all unused code should be cleaned up, preferably along with the addition of the rails pages themselves.

Here they are in a bit more detail:

OPTION 1

• Move over bastion to webpack, but keep bastion in Rails as well
• Move over pages one-by-one in PRs
• In each individual PR, the old rails page’s code will be removed as well
• The PR will also switch the route so it loads the new webpack page ("/content_credentials" now loads the page from the webpack code)
• When all pages are moved over, remove bastion from rails.

OPTION 2 - (its mostly the same, but the difference is the pages aren’t “live” until the end)

• Move over bastion to webpack, but keep bastion in Rails as well.
• Move over pages one-by-one in PRs
• The original rails page’s code stays in master
• The new pages are loaded in a path such as xui/content_credentials
• The rails pages are still used as the ‘live’ user pages until all pages are moved over to webpack
• Once all pages are moved to webpack, modify the routes to use the webpack pages
• Remove all pages and bastion from the Rails side

Both will have the same result, bastion + the bastion katello pages are removed from rails and they now live in webpack. They are just two different ways of getting there.

I’m open to other suggestions as well, these are the best two options I could think of.

[sharvit]

@John_Mitsch what are the benefits of option2? It will make it more complicated while I can’t see the benefit.

[John_Mitsch]

Mostly I’m just trying to lay out different scenarios. I would say the benefit of option 2 is we can make the switch over to the pages in one action and do thorough testing (either manually or automated) before it goes live in a release. It also give us a “switch” that we can flip when we feel ready to, rather than when the changes land in master.

This being said, I’m more in favor of option 1. I think the changes should be incremental and the pages switched over when they are ready. This forces us to pay attention to stability and remove the old code as we go.

[sharvit]

Thanks @John_Mitsch
I would prefer Option 1 as well.