Fixes #27074 - Migrate to @theforeman/vendor and @theforeman/vendor-dev pkg #6605

sharvit · 2019-03-21T16:09:48Z

Uses packages from:
https://github.com/sharvit/foreman-js

theforeman-bot · 2019-03-21T16:09:50Z

There were the following issues with the commit message:

4ee4b81 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

Ron-Lavi · 2019-05-21T11:35:18Z

What's the status of this PR? thanks :)

sharvit · 2019-05-21T12:15:33Z

What's the status of this PR? thanks :)

I'm working on releasing a new version of @theforeman/vendor and then rebasing this PR.
Thanks for caring about vendor @LaViro 👍

theforeman-bot · 2019-05-27T09:25:39Z

There were the following issues with the commit message:

1f6f9df must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

sharvit · 2019-05-28T07:30:57Z

dev-server is working well atm
production build and server working well atm
need to fix some small testing issues

theforeman-bot · 2019-06-06T12:38:52Z

There were the following issues with the commit message:

805807d must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

waldenraines · 2019-06-18T14:02:56Z

I created a redmine issue for this - https://projects.theforeman.org/issues/27074

coveralls · 2019-06-24T10:30:14Z

Coverage increased (+0.2%) to 72.862% when pulling 7fb1165 on sharvit:feature/foreman-vendor into ce287cf on theforeman:develop.

sharvit · 2019-06-24T11:30:20Z

Status Update

I have just released @theforeman/vendor version 0.1.0-alpha.6 and updated this PR to use it.

You can test it by simply run:

rm -rf node_modules/
rm -rf package-lock.json
npm install

# running test should work
npm test

# running development server should work
foreman start

# building and running production should work
RAILS_ENV=production bundle exec rake assets:precompile
RAILS_ENV=production bundle exec rake webpack:compile
RAILS_ENV=production foreman start rails

There is one issue when running `npm run lint`

Yesterday, eslint-plugin-react released version 7.14.0 which break patternfly-react-eslint.
PR opend in patternfly-react to fix it:
patternfly/patternfly-react#2334

sharvit · 2019-06-24T12:41:14Z

There is one issue when running npm run lint

Yesterday, eslint-plugin-react released version 7.14.0 which break patternfly-react-eslint.
PR opend in patternfly-react to fix it:
patternfly/patternfly-react#2334

This is been fixed in eslint-plugin-react version 7.14.1.
The PR in patternfly-react closed.

sharvit · 2019-06-24T17:03:59Z

#6858 created to fix the linting issue

sharvit · 2019-07-09T08:17:40Z

We decided to solve this issue by pushing forward #6748
https://community.theforeman.org/t/rfc-changing-how-we-handle-webpack-building/11491/78?u=sharvit

#6748 is merged now so I'm rebasing this PR and the ruby test error should be resolved.
Thanks @ezr-ondrej @tbrisker

sharvit · 2019-07-09T09:17:15Z

Looks like the failed test is not related?

sharvit · 2019-07-09T15:22:10Z

Update

I have just released @theforeman/vendor version 0.1.0-alpha.9 and this PR use it now.
It fixes the issue that blocked the packaging: [WIP] Add nodejs-theforeman-vendor foreman-packaging#3882
This PR rebased and contains Fixes #21592 - uses headless chrome instead of phantomjs #6748 so the ruby test errors should be fixed now.

Ready for another session of reviews.

app/views/layouts/base.html.erb

config/webpack.config.js

jest.config.js

tbrisker · 2019-07-10T07:50:20Z

config/webpack.config.js

-        collections: true,
-        flattening: true,
-        shorthands: true
-      }),


Reading this plugin's description, i think it may have been the cause of the unexpected hash changes we were seeing in the past:

Create smaller Lodash builds by replacing feature sets of modules with noop, identity, or simpler alternatives.

which is exactly what I discovered when trying to isolate the changes - modules that were being replaced by noop and identity.
Are we still using it in the vendor build? if so, I think we shouldn't - even though we may get a larger build, we want to make all of lodash available to consumers since we can't tell which modules aren't used.

Agree, the new vendor don't use this plugin anymore so it contains the full lodash lib.
Once we stabilize the build, I'm open-minded trying more optimizations.

tbrisker · 2019-07-10T07:53:07Z

script/npm_link_foreman_js.sh

+
+FOREMAN_JS_LOCATION="../${FOREMAN_JS_LOCATION}"
+FOREMAN_JS_PACKAGES_LOCATION="${FOREMAN_JS_LOCATION}/packages"
+FOREMAN_JS_INSTALL_LOCATION="./node_modules/@theforeman"


shouldn't this be "../node_modules/@theforeman"?

No, because you run this script from the root by using npm run foreman-js:link.
See webpack/stories/docs/addingDependencies.md

script/npm_link_foreman_js.sh

webpack/assets/javascripts/bundle.js

sharvit · 2019-07-10T12:58:31Z

@tbrisker can you revisit please?

tbrisker · 2019-07-10T13:40:27Z

script/npm_link_foreman_js.sh

+  exit 1
+fi
+
+FOREMAN_JS_LOCATION="../${FOREMAN_JS_LOCATION}"


this will make it ../../foreman-js with the default, and add a ../ to the path given by the user which would be confusing

imo it will make it easier because you will define FOREMAN_JS_LOCATION relatively to foreman root so you can have:

home/foreman home/foreman-js

So inside foreman you can run:

FOREMAN_JS_LOCATION="../foreman-js" npm run foreman-js:lint

imo it will be confusing if you will need to set FOREMAN_JS_LOCATION relatively to the scripts folder.

didn't you reply below that this script runs from the foreman root because it is executed by npm? in that case ../../foreman-js would be the wrong folder?

I think there is a difference in how the rm and ln commands are working and their parameters.
You can try the script, I have just verified it is working.

I think it is only the second parameter of the ln that need to be relative to root.

ah, i see, it's is because ln creates the link relative to the link location, not to the cwd. you could unite this line with the next but not critical.

tbrisker · 2019-07-10T13:45:27Z

webpack/assets/javascripts/bundle.js

@@ -25,22 +25,8 @@ import * as configReportsModalDiff from './foreman_config_reports_modal_diff';
 import * as classEditor from './foreman_class_edit';
 import * as dashboard from './dashboard';
 import * as spice from './spice';
-import './bundle_datatables';
-import './bundle_lodash';


I may be missing it, where are we making window._ available if the bundle is removed?

Lodash put it there automatically once you require it, the bundle_lodash just replaced the window._ with another leaner object.

tbrisker

Thanks @sharvit ! code looks good, tested and everything seems to work well including several plugins both in development and production mode. This is ready to merge on my side once we figure out the packaging side.

tbrisker · 2019-07-11T08:27:11Z

@sharvit looks like there is a conflict in package.json, please rebase on latest develop

sharvit · 2019-07-11T08:29:17Z

@sharvit looks like there is a conflict in package.json, please rebase on latest develop

This conflict will be solved only by updating the foreman-js project. (and releasing a new @theforeman/vendor to npm)

Notice there is another issue that should get fixed:
theforeman/foreman-packaging#3882 (comment)

tbrisker · 2019-07-11T11:51:19Z

@sharvit besides the packaging issue, this can't be merged right now because of a merge conflict with the current package.json in develop branch. rebasing should fix that.

sharvit · 2019-07-13T10:43:23Z

Thanks @tbrisker

@theforeman/vendor version 0.1.0-alpha.10 has released.
https://github.com/theforeman/foreman-js/compare/@theforeman/vendor@0.1.0-alpha.9...@theforeman/vendor@0.1.0-alpha.10
This PR updated to use the new vendor version.
The babel issue fixed.
This PR as rebased.

sharvit · 2019-07-16T08:58:25Z

Updates

@theforeman/vendor version 0.1.0-alpha.11 has released.
https://github.com/theforeman/foreman-js/compare/@theforeman/vendor@0.1.0-alpha.10...@theforeman/vendor@0.1.0-alpha.11
This PR updated to use the new vendor version.
This PR updated with some scss cleanups

tbrisker

Thanks @sharvit !

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed UI labels Mar 21, 2019

sharvit mentioned this pull request Mar 24, 2019

WIP: @foreman/vendor sub-package #6434

Closed

evgeni mentioned this pull request May 14, 2019

Remove webpack provides/requires from Foreman theforeman/foreman-packaging#3761

Merged

sharvit force-pushed the feature/foreman-vendor branch from 4ee4b81 to 1f6f9df Compare May 27, 2019 09:25

theforeman-bot removed the Waiting on contributor label May 27, 2019

theforeman-bot added the Waiting on contributor label May 27, 2019

sharvit marked this pull request as ready for review May 27, 2019 09:26

sharvit force-pushed the feature/foreman-vendor branch from 1f6f9df to 805807d Compare June 6, 2019 12:38

theforeman-bot removed the Waiting on contributor label Jun 6, 2019

theforeman-bot added the Waiting on contributor label Jun 6, 2019

sharvit mentioned this pull request Jun 18, 2019

Fixes #4 - Build development and production theforeman/foreman-js#6

Merged

13 tasks

sharvit force-pushed the feature/foreman-vendor branch from 805807d to faa0c31 Compare June 24, 2019 09:14

theforeman-bot removed the Waiting on contributor label Jun 24, 2019

sharvit force-pushed the feature/foreman-vendor branch from faa0c31 to bd665a8 Compare June 24, 2019 10:25

sharvit force-pushed the feature/foreman-vendor branch from bd665a8 to 259d7e2 Compare June 24, 2019 12:39

sharvit force-pushed the feature/foreman-vendor branch from 259d7e2 to cde0a5d Compare June 25, 2019 12:08

sharvit force-pushed the feature/foreman-vendor branch from 7fb1165 to 8414bf9 Compare July 9, 2019 15:15

tbrisker reviewed Jul 10, 2019

View reviewed changes

sharvit force-pushed the feature/foreman-vendor branch from 8414bf9 to f8722a9 Compare July 10, 2019 12:57

tbrisker reviewed Jul 10, 2019

View reviewed changes

tbrisker previously approved these changes Jul 11, 2019

View reviewed changes

theforeman-bot removed the Not yet reviewed label Jul 11, 2019

tbrisker added Waiting on Packaging PRs that shouldn't be merged until packaging side is merged and removed Needs testing labels Jul 11, 2019

sharvit dismissed tbrisker’s stale review via 2266dcb July 13, 2019 10:19

sharvit force-pushed the feature/foreman-vendor branch 2 times, most recently from 2266dcb to 2fb0959 Compare July 13, 2019 10:23

sharvit mentioned this pull request Jul 13, 2019

[WIP] Add nodejs-theforeman-vendor theforeman/foreman-packaging#3882

Closed

tbrisker previously approved these changes Jul 14, 2019

View reviewed changes

sharvit dismissed tbrisker’s stale review via c8eb72e July 16, 2019 07:52

sharvit force-pushed the feature/foreman-vendor branch 2 times, most recently from c8eb72e to 123e561 Compare July 16, 2019 08:37

Fixes #27074 - Migrate to @theforeman/vendor pkg

ba5d3e4

sharvit force-pushed the feature/foreman-vendor branch from 123e561 to ba5d3e4 Compare July 16, 2019 08:54

tbrisker approved these changes Jul 16, 2019

View reviewed changes

tbrisker merged commit 0220eaa into theforeman:develop Jul 16, 2019

sharvit deleted the feature/foreman-vendor branch August 5, 2019 15:21

Fixes #27074 - Migrate to @theforeman/vendor and @theforeman/vendor-dev pkg #6605

Fixes #27074 - Migrate to @theforeman/vendor and @theforeman/vendor-dev pkg #6605

Conversation

sharvit commented Mar 21, 2019 • edited

theforeman-bot commented Mar 21, 2019

Ron-Lavi commented May 21, 2019

sharvit commented May 21, 2019

theforeman-bot commented May 27, 2019

sharvit commented May 28, 2019

theforeman-bot commented Jun 6, 2019

waldenraines commented Jun 18, 2019

coveralls commented Jun 24, 2019 • edited

sharvit commented Jun 24, 2019

Status Update

There is one issue when running npm run lint

sharvit commented Jun 24, 2019

There is one issue when running npm run lint

sharvit commented Jun 24, 2019

sharvit commented Jul 9, 2019

sharvit commented Jul 9, 2019

sharvit commented Jul 9, 2019

Update

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharvit commented Jul 10, 2019

Choose a reason for hiding this comment

sharvit Jul 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbrisker left a comment

Choose a reason for hiding this comment

tbrisker commented Jul 11, 2019

sharvit commented Jul 11, 2019 • edited

tbrisker commented Jul 11, 2019

sharvit commented Jul 13, 2019

sharvit commented Jul 16, 2019

tbrisker left a comment

Choose a reason for hiding this comment

sharvit commented Mar 21, 2019 •

edited

coveralls commented Jun 24, 2019 •

edited

There is one issue when running `npm run lint`

There is one issue when running `npm run lint`

sharvit Jul 10, 2019 •

edited

sharvit commented Jul 11, 2019 •

edited