Fixes #19536 - refactor webpack folder #4530

matanwerbner · 2017-05-15T10:24:50Z

Organize the webpack folder better, with seperation for react modules and other foreman modules.

ohadlevy · 2017-05-15T10:44:48Z

webpack/assets/javascripts/bundle.js

@@ -1,26 +0,0 @@
-require('expose?$!expose?jQuery!jquery');


this file can be git mv too?

ohadlevy · 2017-05-15T10:45:24Z

webpack/react_app/storybook/index.js

@@ -0,0 +1,146 @@
+import React from 'react';


ohadlevy · 2017-05-15T11:23:13Z

for other reviewers - it turns out that git mv might not always work, and its up to git to decide if its a new file or moved file.

if we want to fully preserve history, this needs to be done in two commits, otherwise git will create a new file (in cases where a large percentage of the file is changed).

it is possible at times to use git log --follow --find-copies-harder but its not guranteed,.

in this case, I'm not too worried about the (re)created files. other opinions? /cc @tbrisker

tbrisker

Aside from a few inline comments, 2 more general comments:

The reason everything is under assets/javascript is that in the future we may want webpack to manage other assets as well, such as css or fonts. This also separates developer tools such as storybook from the actual assets which are built and delivered to users. TBH I'd even prefer having the tests separated, but I understand the reason they aren't separate due to the directory structure.
This almost completely erases the history of the entire webpack folder for no significant value IMHO. This means that any attempt to understand code in the future will hit the wall of this commit, unless some git magic is used to track the moved files (as github by default doesn't seem to follow the moves).

So I'm afraid, all in all, 👎 from me, unless there is some significant value to this change I'm missing.

tbrisker · 2017-05-15T11:54:54Z

webpack/index.js

+require('expose?ipaddr!ipaddr.js');
+require('jquery.cookie');
+require('expose?JsDiff!diff');
+require('./modules/bundle_flot');


Would it make sense to include the modules directory in the default lookup path for webpack and get rid of all the ./modules/ part?

I dont think webpack default lookup path is a solution for repeating strings. If so it can get huge really fast

tbrisker · 2017-05-15T11:56:56Z

.storybook/config.js

@@ -1,7 +1,7 @@
 import { configure } from '@kadira/storybook';

 function loadStories() {
-  require('../webpack/stories');
+  require('../webpack/react_app/storybook');


Storybook isn't quite part of the react app, I think it makes sense to keep it separate.

Storybook is a development environment for React UI components

it made sense to me to put it under react_app.
webpack is not react dependant, so i wanted to keep the separation between all that is react-related (inside react_app), and other things (modules).

tbrisker · 2017-05-15T12:00:49Z

webpack/react_app/storybook/index.js

@@ -0,0 +1,146 @@
+import React from 'react';


Looks like the original file is also still in your tree.

matanwerbner · 2017-05-15T13:17:31Z

@tbrisker i think if the old javascripts/stylesheets/images structure is irrelevant to webpack, where things are more modular. Anyway the current situation is of redundent folders inside folders that are not doing any good, and overall structure that seems not needed.

ohadlevy · 2017-05-16T12:06:54Z

@tbrisker I've noticed that git blame actually works on github (see https://github.com/matanwerbner/foreman/blame/62f55fbc28c2d15bd2d0d68c7c227f358549ee2b/webpack/react_app/API.js for an example). I find it hard to accept we will not move files in the future because git hub does not use the follow git option.

on the other hand i do understand that if possible we should not move files just for "fun".

to me, this PR is a balance of the two, and i can probably accept both cases...any other opinions?

ohadlevy · 2017-05-16T13:14:37Z

another thought... do we want to include https://github.com/rails/webpacker, that would probably change the structure again? (just for reference I saw ManageIQ/manageiq-ui-classic#1132)

mmoll · 2017-05-16T14:40:04Z

@ohadlevy "Ruby 2.2+" is something that can't be satisfied at the moment for all platforms Foreman does support. Notably Debian 8 (jessie) is coming with 2.1 and 9 (stretch) which would bring 2.3 isn't release, yet.

tbrisker · 2017-05-17T10:01:33Z

@ohadlevy I don't object to moving files if there is a good reason to do so, but this case doesn't quite feel like good enough reason for the most part. (Interesting that blame works even though history doesn't show previous revisions.) This feels more like a disagreement about what the folder structure should look like. I disagree that the existing structure is irrelevant for webpack, it is only if we never want any other assets to be managed by webpack. I don't want to have to change everything again in a year.
I agree there is a good chance we will need to refactor some things in the future again to align ourselves with future rails version that have webpack integration, so it might be better to wait for that rather then make any change.
@mmoll I wonder if there is some way to get around the system ruby version (similar to what we do with SCL on Red Hat based distros)? Eventually we will want to drop support for older ruby versions as our dependencies will drop them and we want to leverage abilities of newer versions.

mmoll · 2017-05-17T10:06:42Z

I wonder if there is some way to get around the system ruby version (similar to what we do with SCL on Red Hat based distros)?

For Ubuntu there's the brightbox PPA, we also used on Ubuntu/precise when Ruby 1.8 was desupported. However, there's no such thing for Debian...

Eventually we will want to drop support for older ruby versions as our dependencies will drop them and we want to leverage abilities of newer versions.

For Ubuntu we'll decide if we desupport older releases or use the PPA again, for Debian we can only drop support... That's why I also mentioned why it's not possible to drop support for Debian 8 at the moment. ;)

ohadlevy · 2017-07-19T19:42:55Z

lets reopen if we decide to introduce webpacker.

theforeman-bot added Needs testing Not yet reviewed labels May 15, 2017

matanwerbner force-pushed the fix_19536_refactor_webpack branch 2 times, most recently from a567bac to c425423 Compare May 15, 2017 10:29

ohadlevy reviewed May 15, 2017

View reviewed changes

webpack/assets/javascripts/bundle.js

@@ -1,26 +0,0 @@

require('expose?$!expose?jQuery!jquery');

Copy link

Member

ohadlevy May 15, 2017

Choose a reason for hiding this comment

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

this file can be git mv too?

ohadlevy reviewed May 15, 2017

View reviewed changes

webpack/react_app/storybook/index.js

@@ -0,0 +1,146 @@

import React from 'react';

Copy link

Member

ohadlevy May 15, 2017

Choose a reason for hiding this comment

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

git mv?

Fixes #19536 - refactor webpack folder

62f55fb

matanwerbner force-pushed the fix_19536_refactor_webpack branch from c425423 to 62f55fb Compare May 15, 2017 11:35

tbrisker requested changes May 15, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels May 15, 2017

ohadlevy closed this Jul 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #19536 - refactor webpack folder #4530

Fixes #19536 - refactor webpack folder #4530

matanwerbner commented May 15, 2017

ohadlevy May 15, 2017

ohadlevy May 15, 2017

ohadlevy commented May 15, 2017

tbrisker left a comment

tbrisker May 15, 2017

matanwerbner May 15, 2017

tbrisker May 15, 2017

matanwerbner May 15, 2017 •

edited

tbrisker May 15, 2017

matanwerbner commented May 15, 2017

ohadlevy commented May 16, 2017

ohadlevy commented May 16, 2017 •

edited

mmoll commented May 16, 2017

tbrisker commented May 17, 2017

mmoll commented May 17, 2017

ohadlevy commented Jul 19, 2017

Fixes #19536 - refactor webpack folder #4530

Fixes #19536 - refactor webpack folder #4530

Conversation

matanwerbner commented May 15, 2017

ohadlevy May 15, 2017

Choose a reason for hiding this comment

ohadlevy May 15, 2017

Choose a reason for hiding this comment

ohadlevy commented May 15, 2017

tbrisker left a comment

Choose a reason for hiding this comment

tbrisker May 15, 2017

Choose a reason for hiding this comment

matanwerbner May 15, 2017

Choose a reason for hiding this comment

tbrisker May 15, 2017

Choose a reason for hiding this comment

matanwerbner May 15, 2017 • edited

Choose a reason for hiding this comment

tbrisker May 15, 2017

Choose a reason for hiding this comment

matanwerbner commented May 15, 2017

ohadlevy commented May 16, 2017

ohadlevy commented May 16, 2017 • edited

mmoll commented May 16, 2017

tbrisker commented May 17, 2017

mmoll commented May 17, 2017

ohadlevy commented Jul 19, 2017

matanwerbner May 15, 2017 •

edited

ohadlevy commented May 16, 2017 •

edited