RFC: Changing how we handle Webpack Building

Development RFCs
61

Updates with @theforeman/vendor@0.1.0-beta.5:

  1. npm run build will produce files with hashes in their names
  2. npm run build will produce a dist/manifest.json with mapping between the chunk name to the files.
  3. Created easier integration with a webpack-plugin
    1. The plugin will copy the dist files the consumer outputPath (e.g. public/webpack)
    2. The plugin will update the consumer manifest.json to include the dist files there (will work with dev-server and production)
    3. The plugin will update the consumer externals configuration.

You can try it with this branch (not have to change plugins to make it work): https://github.com/theforeman/foreman/compare/develop...sharvit:feature/foreman-vendor

1 Like
62

Who releases this and when? Would it make sense to have this live in the foreman git repository?

Currently Iā€™m attempting to do a PoC packaging change for this. I noticed this generates a 34M gzipped cache file which is unacceptable. However, it looks like the current npm2rpm includes all sources into its cache while also fetch the sources from npmjs.org. Thatā€™s a bug we need to get rid of before this is merged. With that solved, it should be much smaller again.

63

Can you elaborate on why this unacceptable?

64

Will this mapping be provided by Foreman core or something each plugin has to implement based on their library usage?

What is the strategy for plugins that have dependencies not in foreman-vendor?

65

Iā€™m guessing 99% of that is the actual NPM package sources which we already have separated out. The cache is intended to only cache the JSON files. Those are only a fraction of that size. Iā€™m probably the one to blame for that regression in npm2rpm because Iā€™m pretty sure it wasnā€™t always like that.

66

Status update on the packaging side. I have a draft PR with a bundle file that builds locally. Havenā€™t tried to build the Foreman RPM with the right sources yet.

https://github.com/theforeman/foreman-packaging/pull/3525

It does make clear how much duplication there is. Better than before. For example, itā€™s clear thereā€™s a react-bootstrap 0.32.1 and 0.32.4 included. That doesnā€™t sounds right to me.

In the past I did patch around this in a few places and sent patches upstream. Itā€™d be good if more people took the effort. It has many benefits, like smaller npm installs and fewer bugs.

1 Like
67

There are many options, my suggestion:

Have a separate git repository theforeman/foreman-frontend where we can put multiple sub-packages.
atm we can put there @theforeman/vendor and @theforeman/test-utils from https://github.com/sharvit/react-redux-test-utils (we had a discussion we want it under theforeman org)
And I believe we have a case for more sub-packages to create in the future.
patternfly-react do the same while they are managing multiple projects in one repo using lerna.
With lerna we can have separate lifecycles and automated releases for each sub-package.

The reason I donā€™t want it to be apart of core because it will lead to lots of confusions about how staff links together when they live together in the same repo but got consumed from npm.
If you will not use npm and you will link them locally, you will lose the versioning benefit of the sub-packages, then you have the issue of how plugins should consume those packages.

atm I donā€™t think we should get that forward with automations, I think we should tag releases manually and let Travis deploy npm and rpm (I donā€™t know much about rpms) when we create a new release commit and tag manually after X prs/days.
right now my repo use travis to deploy to npm when I create a new git tag and update the package.json.

68

Because atm core build the plugins, the plugins donā€™t need to change anything.
But core donā€™t really need to implement anything, @theforeman/vendor provides a webpack-package that do the mapping for you. All you need to do is adding this plugin to you webpack config:

// webpack.config.js
const ForemanVendorPlugin = require('@theforeman/vendor/webpack.plugin');

module.exports = {
  entry: { ... },
  output: { ... },
  module: { ... },
  plugins: [
    new ForemanVendorPlugin(),
    ...
  ],
};

Once plugins would be able to build themselves (and we should aim there), consuming @theforeman/vendor mapping would be easy as that.
Those mapping will provide to core and plugins all the mappings included in https://github.com/sharvit/foreman-vendor/blob/master/lib/webpack-vendor.js

While this is the only change you will need to do to run dev-server and build prod/dev, there are other changes you will need to do in your project as a consumer of @theforeman/vendor so you can run tests, storybook and linting and those changes described in the readme file:
https://github.com/sharvit/foreman-vendor

69

If plugin (or core) depend on a package include in foreman-vendor it will ignore the local package and will use the package included in foreman-vendor.
If a package not included in foreman-vendor it will compile this package into the core/plugin bundle file.

70

We might be able to reduce it, you need different packages for different use-cases.

Build the foreman-vendor dist files
Needs all the packages, once done, you have dist files in npm.

Run foreman dev-server, lint, storybook, testing
Needs all the nonDev dependencies in foreman-vendor package.json

Build foreman/plugins production/dev bundles
Needs only one package from the foreman-vendor package.json, copy-webpack-plugin.

Can we reduce build sizes around it?

71

To be perfectly clear: currently we have all sources (babel-x.y.z.tgz, react-x.y.z.tgz) listed in the spec file. We also have a tarball that contains the cached responses from npm (PACKAGE-registry-x.y.z.tgz) which we check into git directly. It turns out that also contains the sources (babel-x.y.z.tgz, ā€¦). This duplication doesnā€™t serve any purpose and only blows up our git repository size. I think a patch that clears the duplication and reuses the sources should be easy to come up with.

72

A quick test removing the actual sources reduced the cache size from 34M to 14M. Will proceed with that further. Looks like weā€™ve been duplicating that for quite some time but this ridiculous size made it obvious.

I do think itā€™d be good to have a package that you can add to your devRequires and one to your requires. From the packaging side that would greatly improve our life.

2 Likes
73

Agree, I will work on a separation between @theforeman/vendor and @theforeman/vendor-dev :+1:

1 Like
74

Updates

Created a new monorepo foreman-js using lerna with the following packages:

Created a new draft PR that integrates with @theforeman/vendor and @theforeman/vendor-dev:

2 Likes
75

I may have asked this before. I had a general question regarding the overall architecture of UI components and how plugins are handled.

  1. Could the navigation be split intoā€™s own .js file with itā€™s own bound dependencies (e.g. nav-vendor.js) that is loaded on all pages?
  2. Could the JS that is loaded per page be reduced to a set of JS for the navigation, and then any JS the plugin (or core) page being loaded requires to function?
  3. Could plugins be built against and include their own vendor-.js (or included into their plugin.js) such that the JS for a given plugin is independently built and loaded thus isolating it from the current dependency chain?

This in my estimation would give plugins back the flexibility they need and reduce the burden on the build system that we encounter to date. This may in part require previously proposed solutions but it feels like there has got to be a way to arrive at a more independent architecture for plugins to prevent deadlock.

76

Status update, see:

77

I gave it a try and it seems to be working fine, here is a breakdown of the assets foreman compiles

Before this Patch

image
image1918Ɨ948 255 KB

After this patch

image
image1912Ɨ948 294 KB

if you want to give a try you can use Quay to try it out.

78

Update from a 03.07.19 meeting between @tbrisker @ekohl @evgeni @amirfefer and I about this topic

  1. Working on fixing the issue that blocks the packaging https://github.com/theforeman/foreman-packaging/pull/3882
    The issue is the vendor uses vendor-core in the plugin it delivers to foreman.
    The approach will be to update the build process of the vendor so it will have the file it uses from vendor-core internally.
    @sharvit

  2. Continue the packaging work on @theforeman/vendor.
    Blocked by #1
    @ekohl @evgeni

  3. Fix the ruby integration test errors: https://ci.theforeman.org/job/test_develop_pr_core/1282/database=postgresql-integrations,ruby=2.5,slave=fast/testReport/junit/(root)/AboutIntegrationTest/test_0001_about_page/
    All the integration test fails atm but works well when using the chrome driver.

    1. Push forward the work on https://github.com/theforeman/foreman/pull/6748
      @ezr-ondrej @tbrisker
    2. Rebase the migration PR in core to use https://github.com/theforeman/foreman/pull/6748 so we can verify it is actually solving the test errors.
      @sharvit

Ping me if you want to join the next meeting.

Thanks!

79

Update from a 10.07.19 meeting between @tbrisker @ekohl @evgeni and I about this topic

Done last week

  1. @theforeman/vendor version 0.1.0-alpha.9 released with https://github.com/theforeman/foreman-js/pull/13

  2. The ruby test errors got fixed by merging and rebasing @ezr-ondrej PR that switches to headless chrome based testing https://github.com/theforeman/foreman/pull/6748
    Thanks @ezr-ondrej, @tbrisker

Goals for the next week

  1. The packaging team will continue the effort packaging @theforeman/vendor version 0.1.0-alpha.9 and produce a koji build. @tbrisker @ekohl @evgeni

  2. Finish the work over https://github.com/theforeman/foreman/pull/6605
    The goal is to merge until the end of the next week (18.07).
    @sharvit @tbrisker

Cheers!

1 Like
80

This has been merged:

https://github.com/theforeman/foreman/commit/0220eaac7cbfc419aa0570a53308d9b2d25b72e5

RPM packaging has been updated, but plugins need to be rebuilt. Expect some breakages in nightlies over the next few days.

This also means all PRs that touch package.json need to be rebased. Iā€™ll leave it to @sharvit to instruct developers on this :slight_smile:

1 Like