Skip to content
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

Fixes #28270 - consume babel from @theforeman/builder and @theforeman/env #7178

Merged
merged 3 commits into from Dec 3, 2019

Conversation

sharvit
Copy link
Contributor

@sharvit sharvit commented Nov 14, 2019

The babel configuration moved to @theforeman/env so it can be shared with core and plugins.

@theforeman-bot
Copy link
Member

Issues: #28270

package.json Outdated Show resolved Hide resolved
@sharvit sharvit marked this pull request as ready for review November 17, 2019 15:38
@xprazak2
Copy link
Contributor

Packaging: theforeman/foreman-packaging#4348

@xprazak2 xprazak2 added Waiting on Packaging PRs that shouldn't be merged until packaging side is merged and removed Needs testing Not yet reviewed labels Nov 18, 2019
package.json Outdated
@@ -34,22 +34,13 @@
"@storybook/addon-storysource": "^3.4.12",
"@storybook/react": "~3.4.12",
"@storybook/storybook-deployer": "^2.0.0",
"@theforeman/vendor-dev": "^3.0.0",
"@theforeman/env": "^3.1.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

amirfefer
amirfefer previously approved these changes Nov 18, 2019
Copy link
Member

@amirfefer amirfefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! works as expected!
Thanks @sharvit !

Why did you add the redux update commit?

@sharvit
Copy link
Contributor Author

sharvit commented Nov 19, 2019

Why did you add the redux update commit?

Because I need to use vendor v3 while develop uses vendor v1, the redux commit updates the vendor to v2 so it makes sense using it.

adamruzicka
adamruzicka previously approved these changes Nov 20, 2019
Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work nicely together with REX and tasks

@sharvit sharvit dismissed stale reviews from adamruzicka and amirfefer via 1062f27 November 21, 2019 15:55
@sharvit sharvit changed the title Fixes #28270 - consume babel from @theforeman/env Fixes #28270 - consume babel from @theforeman/babel Nov 21, 2019
package.json Outdated
@@ -34,22 +34,14 @@
"@storybook/addon-storysource": "^3.4.12",
"@storybook/react": "~3.4.12",
"@storybook/storybook-deployer": "^2.0.0",
"@theforeman/vendor-dev": "^1.7.0",
"@theforeman/babel": "^3.2.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a packaging PR

"babel-register": "^6.9.0",
"babel-eslint": "^10.0.0",
"babel-jest": "^24.9.0",
"babel-loader": "^8.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

babel-loader needs packaging PR.
(it is in the scope of webpack, not babel, as it's a webpack plugin)

@sharvit
Copy link
Contributor Author

sharvit commented Nov 21, 2019

Updates about packaging: @evgeni @ekohl @ehelms @tbrisker

The last effort migrating to vendor v3 blocked because we found out we need to do packaging for too many babel-<something> modules while we already have a solution for grouping them in one package to simplify the packaging process.

Therefore I created the @theforeman/babel module that contains all the babel modules and group them together.

To merge this PR, we need:

  1. Update @theforeman/vendor in packaging to v3.2.0
  2. Remove all the babel staff from packaging
  3. Add @theforeman/babel to the packaging
  4. Remove all the babel related things from packaging (except babel-loader)
  5. Update the babel-loader in packaging to >=v8
    (Did I forget something?)

I didn't want to do all those changes by myself because I am not that familiar with the packaging and I guess they should all be done in 1 PR. Can I get some help here from the packaging team?

@evgeni
Copy link
Member

evgeni commented Nov 22, 2019

This sounds sensible to me, but I'd like another ack from @ehelms or @ekohl.
We then basically end up with a big @theforeman/babel RPM that contains all the babel things and can carry on. Technically we don't need to remove the old babel stuff from packaging, but we should double-check that after merging these changes to plugins nothing more pulls that in.

@ekohl
Copy link
Member

ekohl commented Nov 22, 2019

I can only agree with what @evgeni said.

amirfefer and others added 2 commits November 26, 2019 16:29
@evgeni
Copy link
Member

evgeni commented Nov 27, 2019

Started builder packaging in theforeman/foreman-packaging#4413

@evgeni
Copy link
Member

evgeni commented Nov 27, 2019

babel-loader: theforeman/foreman-packaging#4416

@evgeni
Copy link
Member

evgeni commented Nov 28, 2019

so after building vendor 3.3.0, builder 3.3.0 and babel-loader 8.0.6, I get the following when trying to build foreman:

ERROR in ./webpack/assets/javascripts/bundle.js
Module build failed: Error: Cannot find module '@babel/core'
 babel-loader@8 requires Babel 7.x (the package '@babel/core'). If you'd like to use Babel 6.x ('babel-core'), you should install 'babel-loader@7'.
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
    at Function.Module._load (internal/modules/cjs/loader.js:562:25)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.<anonymous> (/opt/theforeman/tfm/root/usr/lib/node_modules/babel-loader/lib/index.js:10:11)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at loadLoader (/opt/theforeman/tfm/root/usr/lib/node_modules/webpack/node_modules/loader-runner/lib/loadLoader.js:18:17)
    at iteratePitchingLoaders (/opt/theforeman/tfm/root/usr/lib/node_modules/webpack/node_modules/loader-runner/lib/LoaderRunner.js:169:2)
    at runLoaders (/opt/theforeman/tfm/root/usr/lib/node_modules/webpack/node_modules/loader-runner/lib/LoaderRunner.js:365:2)
    at NormalModule.doBuild (/opt/theforeman/tfm/root/usr/lib/node_modules/webpack/lib/NormalModule.js:182:3)
    at NormalModule.build (/opt/theforeman/tfm/root/usr/lib/node_modules/webpack/lib/NormalModule.js:275:15)
    at Compilation.buildModule (/opt/theforeman/tfm/root/usr/lib/node_modules/webpack/lib/Compilation.js:157:10)
    at moduleFactory.create (/opt/theforeman/tfm/root/usr/lib/node_modules/webpack/lib/Compilation.js:460:10)
    at factory (/opt/theforeman/tfm/root/usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:243:5)
    at applyPluginsAsyncWaterfall (/opt/theforeman/tfm/root/usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:94:13)
    at /opt/theforeman/tfm/root/usr/lib/node_modules/webpack/node_modules/tapable/lib/Tapable.js:268:11
    at NormalModuleFactory.params.normalModuleFactory.plugin (/opt/theforeman/tfm/root/usr/lib/node_modules/webpack/lib/CompatibilityPlugin.js:52:5)
    at NormalModuleFactory.applyPluginsAsyncWaterfall (/opt/theforeman/tfm/root/usr/lib/node_modules/webpack/node_modules/tapable/lib/Tapable.js:272:13)
    at resolver (/opt/theforeman/tfm/root/usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:69:10)
    at process.nextTick (/opt/theforeman/tfm/root/usr/lib/node_modules/webpack/lib/NormalModuleFactory.js:196:7)
    at process._tickCallback (internal/process/next_tick.js:61:11)
error: Bad exit status from /var/tmp/rpm-tmp.P0dUQ2 (%build)
    Bad exit status from /var/tmp/rpm-tmp.P0dUQ2 (%build)

@evgeni
Copy link
Member

evgeni commented Nov 28, 2019

actually adding @babel/core to the buildroot gives me

ERROR in ./webpack/assets/javascripts/bundle.js
Module build failed: Error: /builddir/build/BUILD/foreman-1.25.0-develop/.babelrc.js: Error while loading config - Cannot find module '@theforeman/env/babel'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
    at Function.resolve (internal/modules/cjs/helpers.js:33:19)
    at Object.<anonymous> (/builddir/build/BUILD/foreman-1.25.0-develop/.babelrc.js:4:13)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at readConfigJS (/opt/theforeman/tfm/root/usr/lib/node_modules/@babel/core/lib/config/files/configuration.js:190:26)
    at cachedFunction (/opt/theforeman/tfm/root/usr/lib/node_modules/@babel/core/lib/config/caching.js:32:19)
    at readConfig (/opt/theforeman/tfm/root/usr/lib/node_modules/@babel/core/lib/config/files/configuration.js:162:44)
    at names.reduce (/opt/theforeman/tfm/root/usr/lib/node_modules/@babel/core/lib/config/files/configuration.js:128:20)
    at Array.reduce (<anonymous>)
    at loadOneConfig (/opt/theforeman/tfm/root/usr/lib/node_modules/@babel/core/lib/config/files/configuration.js:125:24)
    at findRelativeConfig (/opt/theforeman/tfm/root/usr/lib/node_modules/@babel/core/lib/config/files/configuration.js:100:16)
    at buildRootChain (/opt/theforeman/tfm/root/usr/lib/node_modules/@babel/core/lib/config/config-chain.js:113:39)
    at loadPrivatePartialConfig (/opt/theforeman/tfm/root/usr/lib/node_modules/@babel/core/lib/config/partial.js:85:55)
    at Object.loadPartialConfig (/opt/theforeman/tfm/root/usr/lib/node_modules/@babel/core/lib/config/partial.js:110:18)
    at Object.<anonymous> (/opt/theforeman/tfm/root/usr/lib/node_modules/babel-loader/lib/index.js:144:26)
    at Generator.next (<anonymous>)
    at asyncGeneratorStep (/opt/theforeman/tfm/root/usr/lib/node_modules/babel-loader/lib/index.js:3:103)
    at _next (/opt/theforeman/tfm/root/usr/lib/node_modules/babel-loader/lib/index.js:5:194)
    at /opt/theforeman/tfm/root/usr/lib/node_modules/babel-loader/lib/index.js:5:364
    at new Promise (<anonymous>)
    at Object.<anonymous> (/opt/theforeman/tfm/root/usr/lib/node_modules/babel-loader/lib/index.js:5:97)
    at Object._loader (/opt/theforeman/tfm/root/usr/lib/node_modules/babel-loader/lib/index.js:224:18)
    at Object.loader (/opt/theforeman/tfm/root/usr/lib/node_modules/babel-loader/lib/index.js:60:18)
    at Object.<anonymous> (/opt/theforeman/tfm/root/usr/lib/node_modules/babel-loader/lib/index.js:55:12)
error: Bad exit status from /var/tmp/rpm-tmp.M5dlPM (%build)
    Bad exit status from /var/tmp/rpm-tmp.M5dlPM (%build)

- Consume prod babel from @theforeman/builder/babel
- Consume dev  babel from @theforeman/env/babel
@sharvit
Copy link
Contributor Author

sharvit commented Dec 2, 2019

actually adding @babel/core to the buildroot gives me

It should work now :)
Can you please try again?

@evgeni
Copy link
Member

evgeni commented Dec 2, 2019

So I tried with bec5ad8 (that was the version that was using 3.3.1) and it seemed to be fine. I needed to build babel-loader, @babel/core and obviously vendor and builder. 👍

I tried clicking around in the UI and saw no obvious errors, also nothing in the Console log.

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packaging: ACK

@sharvit
Copy link
Contributor Author

sharvit commented Dec 2, 2019

Thanks @evgeni 👍

The Travis errors are unrelated: https://projects.theforeman.org/issues/28391

@evgeni
Copy link
Member

evgeni commented Dec 3, 2019

@sharvit makes sense, cool.

can someone please ACK this from the JS/UX perspective please?

Copy link
Member

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sharvit, tested LGTM 👍

Copy link
Member

@amirfefer amirfefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storybook is broken after the upgrade with this error:

ERROR in ./.storybook/config.js
Module build failed: Error: [BABEL] /home/afeferku/git/foreman/.storybook/config.js: Cannot find module
 '@theforeman/vendor-core' (While processing: 
"/home/afeferku/git/foreman/node_modules/@theforeman/env/babel/index.js")

EDIT: I was on an a previous version, after rebuilding it works well

Copy link
Member

@amirfefer amirfefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

foreman and storybook work well 👍
tested in develop and production

Thanks @sharvit !

@amirfefer amirfefer merged commit ce12cd9 into theforeman:develop Dec 3, 2019
@sharvit
Copy link
Contributor Author

sharvit commented Dec 3, 2019

Thanks @amirfefer @LaViro @evgeni @xprazak2 @adamruzicka 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI Waiting on contributor Waiting on Packaging PRs that shouldn't be merged until packaging side is merged
Projects
None yet
8 participants