Fixes #21203 - Move from webpack-rails to webpacker #5033

sharvit · 2017-11-27T14:57:59Z

Move to webpacker

webpack-rails is no longer being maintained so we need to switch to webpacker.

Important Notes

webpacker come preconfigured with almost all the webpack configurations we need so we can remove a big amount of the webpack configurations from our codebase.
webpacker come together with with the plugins and the loaders as npm dependencies so we can remove them from our package.json file.
webpacker has a dev server that does not support config variables from the cli but does support environment variables as config in the form WEBPACKER_DEV_SERVER_<OPTION>. Please note that these environment variables will always take precedence over the ones already set in the configuration file.

Before

./bin/webpack-dev-server --https
# or
WEBPACK_OPTS='--https --key /path/to/key --cert /path/to/cert.pem --cacert /path/to/cacert.pem' foreman start
# or
WEBPACK_OPTS='--https --key /path/to/key --cert /path/to/cert.pem --cacert /path/to/cacert.pem' ./script/foreman-start-dev

After

WEBPACKER_DEV_SERVER_HTTPS=true ./bin/webpack-dev-server
# or
WEBPACKER_DEV_SERVER_HTTPS=true \
WEBPACKER_DEV_SERVER_KEY=/path/to/key \
WEBPACKER_DEV_SERVER_CERT=/path/to/cert.pem \
WEBPACKER_DEV_SERVER_CACERT=/path/to/cacert.pem \
foreman start
# or
WEBPACKER_DEV_SERVER_HTTPS=true \
WEBPACKER_DEV_SERVER_KEY=/path/to/key \
WEBPACKER_DEV_SERVER_CERT=/path/to/cert.pem \
WEBPACKER_DEV_SERVER_CACERT=/path/to/cacert.pem \
./script/foreman-start-dev

Checkout the changes in Procfile and scripts/foreman-start-dev.
Need to ensure no one is using the $WEBPACK_OPTS.

Webpacker using yarn as a package manager instead of npm.
yarn using a lock file (like bundler do) name yarn.lock.
package.json still functioning as normal

Install yarn

https://yarnpkg.com/en/docs/install

Usaging yarn

https://yarnpkg.com/en/docs/usage

I removed the webpack-stats-plugin, this plugin has only 2 stars and I don't see any reason to use it. Let me know if you disagree.
Webpack configurations is now under config/webpack/${env-name}.js.
The old entry file was webpack/assets/javascripts/bundle.js, this file still exists at the same location but src/javascript/packs/application.js is the new entry file which only imports the bundle.js. All the entry files should live under app/javascript/packs and will automatically become entry files.
In order to include the entry file in the erb file you should use:

<%= stylesheet_pack_tag 'pack_name' %>
<%= javascript_pack_tag 'pack_name' %>

Need to replace them in plugins
I would like to continue with another pr to completely change the file structure of webpack to be:

app/javascript:
  ├── packs:
  |   # only webpack entry files here
  │   └── application.js
  └── src:
      ├── compute_resource:
      ├── react_app:
      ├── services:
      ├── foreman_editor.js
      └── foreman_hosts.js

So everything from /webpack will move to app/javascript/src. Let me know what do you think about it!

I removed the webpack configurations (enabled, use-https) from the settings.yaml file.

webpack will always be enable when RAILS_ENV=development.
(will only use content-security-policy to allow comunication to the webpack-dev-server)
To run webpack-dev-server over https use WEBPACKER_DEV_SERVER_HTTPS=true. This setting is also exist in config/webpacker.yml.

To precompile webpack assets you can run:

./bin/webpack
# or
bundle exec rake webpacker:compile
# or together with the assets pipeline
bundle exec rails assets:precompile

Webpack compilation will be different based on your RAILS_ENV

All the environment variables getting loaded into the process.env. More info: https://github.com/rails/webpacker/blob/master/docs/env.md
postcss is configured out of the box together with cssnext and configured via .postcssrc.yml.
app/helpers/reactjs_helper.rb contain the mount_react_component method together with some more methods. I never find a place that using those methods. I felt free to remove them. Correct me if I am wrong!
We can move forward and use webpacker-react, the benefits are:

webpacker-react has it's own root components registry so we will be able to remove our registry implementation.

webpacker-react can replace our implementation of mount_react_component.

use from inside a view

<%= react_component('Hello', name: 'React') %>

use from controller

class PageController < ApplicationController
    def main
      render react_component: 'Hello', props: { name: 'React' }
    end
end

What do you think about migrating to webpacker-react? Will prefer to do so in a seperate pr

TODO

Verify that foreman plugins that use webpack still works as usual (katello)
Ensure no one is using the $WEBPACK_OPTS and remove it from all the docs

theforeman-bot · 2017-11-27T14:58:07Z

Issues: #21203

sharvit · 2017-11-27T15:17:13Z

lib/foreman/silenced_logger.rb

+    # Allow webpack to run logger.tagged
+    def tagged(*tags)
+      yield self
+    end


It seems that our logger is outdated.
I had to use this "hack" because webpacker is running:

initializer "webpacker.logger" do config.after_initialize do if ::Rails.logger.respond_to?(:tagged) Webpacker.logger = ::Rails.logger else Webpacker.logger = ActiveSupport::TaggedLogging.new(::Rails.logger) end end end Webpacker.logger.tagged("Webpacker") { compiler.compile }

should we send an upstream patch to support this on the logging gem?

I am not sure about it, check out this file:
https://github.com/theforeman/foreman/blob/develop/lib/foreman/logging.rb#L53-L56

Why do we need so many loggers? Is it possible that this function tries to achieve the same behavior as tagged?

sharvit · 2017-11-27T15:33:20Z

Katello most likely to fail because of this file:
https://github.com/Katello/katello/blob/master/app/views/katello/layouts/react.html.erb

Those lines:

<% content_for(:stylesheets) do %>
    <%= stylesheet_link_tag *webpack_asset_paths('katello', :extension => 'css'), "data-turbolinks-track" => true %>
<% end %>

<% content_for(:javascripts) do %>
    <%= javascript_include_tag *webpack_asset_paths('katello', :extension => 'js'), "data-turbolinks-track" => true, 'defer' => 'defer' %>
<% end %>

Needs to change to:

<% content_for(:stylesheets) do %>
    <%= stylesheet_pac_tag 'katello', "data-turbolinks-track": true %>
<% end %>

<% content_for(:javascripts) do %>
    <%= javascript_pac_tag 'katello', "data-turbolinks-track": true, 'defer':  'defer' %>
<% end %>

xprazak2 · 2017-11-28T07:40:51Z

app/helpers/reactjs_helper.rb

@@ -4,31 +4,4 @@ def mount_react_component(name, selector, data = [])
      "$(tfm.reactMounter.mount('#{name}', '#{selector}', #{data}));".html_safe
    end
  end
-
-  def webpacked_plugins_js_for(*plugin_names)


These helpers were introduced recently and are meant to be used by plugins - they make sure files from plugins are loaded in the page. I believe we want to keep the functionality.

xprazak2 · 2017-11-28T07:56:20Z

Webpacker is using yarn as a package manager instead of npm.

Are there plans to move to yarn completely in the future?

I set the release in Redmine to 1.18. I would like to ask not to merge this before 1.17 has branched since there are changes here that will significantly affect packaging. Thanks.

sharvit · 2017-11-28T18:40:30Z

Thanks for the feedback @xprazak2!
I bring back the helpers, migrate them to webpacker and fixed some testing issues.

About yarn, i belive the plan should be to move to yarn completly since it comes with some benefits and it is being adopted by rails 5.

ohadlevy · 2017-11-28T21:05:48Z

.babelrc

+      {
+        "modules": false,
+        "targets": {
+          "browsers": "> 1%",


why are we changing the defaults? are those webpacker defaults?

ohadlevy · 2017-11-28T21:06:15Z

.postcssrc.yml

@@ -0,0 +1,3 @@
+plugins:
+  postcss-smart-import: {}


what is this file for?

postcss is configured out of the box together with cssnext and configured via .postcssrc.yml.

ohadlevy · 2017-11-28T21:09:45Z

config/webpacker.yml

+  dev_server:
+    https: false
+    host: localhost
+    port: 3035


just wondering why introduce a different port?

No reason, it is just the webpacker default port, can change it back.

ohadlevy · 2017-11-28T21:10:21Z

lib/foreman/silenced_logger.rb

+    # Allow webpack to run logger.tagged
+    def tagged(*tags)
+      yield self
+    end


should we send an upstream patch to support this on the logging gem?

ohadlevy · 2017-11-28T21:11:07Z

lib/tasks/webpack_try_compile.rake

    rescue => e
-      puts "WARNING: `rake webpack:compile` failed to run. This is only important if running integration tests. (cause: #{e})"
+      puts "WARNING: `rake webpacker:compile` failed to run. This is only important if running integration tests. (cause: #{e})"


does it make sense to support also the old task name, so packaging/scripting do not need to update for this change?

It does make sense, what about supporting the old task names with a deprecated warning?
We can keep it for a few releases and then stop to support it.

I think we need a global approach about how to deprecate methods.

ohadlevy · 2017-11-28T21:11:38Z

package.json

    "babel-polyfill": "^6.26.0",
    "brace": "^0.10.0",
    "c3": "^0.4.11",
-    "datatables.net": "~1.10.12",
-    "datatables.net-bs": "~1.10.12",
+    "coffeescript": "^2.0.2",


why do we need coffescript?

Well, it is a peerDependency in webpacker:
https://github.com/rails/webpacker/blob/master/package.json#L55

I have tried to remove it and it doesn't affect anything but at the end I still kept it.

Do you think I should open an issue in webpacker? or maybe just get rid of it and live in peace with the warning?

ohadlevy · 2017-11-28T21:12:14Z

package.json

-    "datatables.net-bs": "~1.10.12",
+    "coffeescript": "^2.0.2",
+    "datatables.net": "1.10.15",
+    "datatables.net-bs": "1.10.15",


were you able to use datatables with yarn? I got DataTables/Dist-DataTables-Bootstrap#2

I had some problems and couldn't really understand the reason for that.
At the end, I dived into the code and found some solution/hack but I will give it a second look while considering your issue.

I am not sure what causing it, it seems the versions are fine at the yarn.lock.
If upgrade to 1.10.16 i am getting errors.

i think this is exactly DataTables/Dist-DataTables-Bootstrap#2

OK, i checked it again and npm resloving 1.10.15 while yarn resolving 1.10.16.
When usingnpm i don't need to change the package.json and the bundle_datatables.js.

Do you have any suggestions?

read the PR with reference to yarn issue..... tl/dr - merge the pr :)

you mean wait/put pressure on datatables.net-bs to merge you pr?
or switch back to npm? I didn't realized foreman used to work with yarn before.

@ohadlevy - updating jquery to v3 solving the issue!
I am working on a separate pr to update it.

Anyway, we will be able to replace those data tables soon after #5024 will merge.

ohadlevy · 2017-11-28T21:12:32Z

package.json

@@ -70,9 +55,9 @@
    "patternfly-react": "^0.8.1",
    "prop-types": "^15.6.0",
    "raf": "^3.4.0",
-    "react": "^16.0.0",
+    "react": "^16.1.1",


probably should be done in a different pr.

After rethinking about it, we introducing yarn so it will get saved to my yarn.lock as 16.1.1 anyway.
Would you like to force it to be 16.0.0 on the yarn.lock file?

ohadlevy · 2017-11-28T21:13:23Z

test/helpers/reactjs_helper_test.rb

@@ -3,8 +3,8 @@
 class ReactjsHelperTest < ActionView::TestCase
  include ReactjsHelper

-  def webpack_asset_paths(bundle_name, opts)
-    ["<script src=\"https://foreman.example.com:3808/webpack/#{bundle_name}.js\"></script>"]
+  def javascript_pack_tag(bundle_name, opts)


does this works on dev? i thought it should use the different port to access the dev-server?

It's a test file! the meaning is only to cancel the default behavior of webpacker so it won't actually search inside the manifest.json and load an entry which doesn't really exist.

It won't even add it to the html.

ohadlevy · 2017-11-28T21:14:34Z

webpack/assets/javascripts/bundle_datatables.js


+$.fn.DataTable = dataTable;
+
+require('datatables.net-bs');


why not import?
if this solve the original issue i was facing with dataTable, maybe it makes sense to send this as a different pr regardless.

I thought it should be synced because datatables.net-bs using the $.fn.DataTable.
I have test it again and it works fine with import 👍

sharvit · 2017-11-29T16:13:01Z

@ohadlevy in order to make jenkins build work, we will need to update the jenkins build script.
How can it be done?

xprazak2 · 2017-11-30T08:18:12Z

Jobs on jenkins are defined using jenkins job builder from our infra repo

sharvit · 2017-11-30T08:58:10Z

Thanks @xprazak2 👍

tstrachota · 2018-01-04T14:32:46Z

We can move forward and use webpacker-react, the benefits are:

How does webpacker-react's registry and mounting helper work with HOCs? We're currently using Provider to inject redux store and there's potential for adding more (e.g. IntlProvider):
https://github.com/theforeman/foreman/blob/develop/webpack/assets/javascripts/react_app/common/MountingService.js#L30

If this isn't solved in webpacker-react I'd keep our current solution.

sharvit · 2018-01-04T15:21:45Z

@tstrachota good point, it might be an issue, all the example i saw warping root components with Provider manually. (Inside the component)

But maybe we would be able to use some hybrid solution when we override the mounting service or the registry service.

himdel · 2018-08-19T14:24:02Z

Is this still in the works?

Trying to figure out if I can do anything in #5867 to not break stuff for you :).

sharvit · 2018-08-20T08:13:55Z

@himdel Feel free to break everything 👍
I got tones of rebases to do anyway and I personally think the webpack-4 is more exciting.

mmoll · 2018-09-01T13:43:49Z

While I don't see much pressue currently, this migration needs to be done somewhen for the sake of future-safety (Rails >5.2).

ohadlevy · 2018-10-08T12:30:48Z

i believe we are no longer targeting migration to webpacker, as it doesn't support rails engines...

johnpmitsch · 2018-10-12T17:58:26Z

Are there any other plans here? It seems like using an unmaintained gem for webpack will become an issue eventually. Should we file RFEs with wepbacker and/or ask for their opinion?

ekohl · 2018-10-13T16:01:00Z

There's an issue open to do so. One bigger implication is that it's using yarn instead of NPM. There used to be an issue with that in our JS stack as well. Maybe we've moved past that by now.

johnpmitsch · 2018-10-15T13:26:16Z

@ekohl thanks, I found the issue here if anyone else is curious

iNecas · 2019-01-07T07:53:17Z

Seems like the support for engines has been added to webpacker just recently (rails/webpacker#1836, docs here). It would be god to reopen this and make it working.

We've started hitting limitations of using non-maintained dependency for example here #6380

ares · 2019-01-07T11:43:26Z

+1 to that

theforeman-bot added Needs testing Not yet reviewed labels Nov 27, 2017

sharvit commented Nov 27, 2017

View reviewed changes

xprazak2 reviewed Nov 28, 2017

View reviewed changes

dLobatog changed the title ~~Fixes #21203 - Move from webpack-rails to webacker~~ Fixes #21203 - Move from webpack-rails to webpacker Nov 28, 2017

sharvit force-pushed the feature/21203-webpacker branch from 52d7245 to e31ff60 Compare November 28, 2017 18:31

ohadlevy reviewed Nov 28, 2017

View reviewed changes

sharvit force-pushed the feature/21203-webpacker branch 2 times, most recently from c278495 to 0d8285e Compare November 29, 2017 16:12

Fixes #21203 - Move from webpack-rails to webacker

85b6a5a

sharvit force-pushed the feature/21203-webpacker branch from 0d8285e to 85b6a5a Compare December 4, 2017 10:47

sharvit mentioned this pull request Dec 14, 2017

Fixes #21846 - React turbolinks fix #5083

Merged

ohadlevy closed this Oct 8, 2018


		$.fn.DataTable = dataTable;

		require('datatables.net-bs');

Fixes #21203 - Move from webpack-rails to webpacker #5033

Fixes #21203 - Move from webpack-rails to webpacker #5033

Conversation

sharvit commented Nov 27, 2017 • edited Loading

Move to webpacker

Important Notes

Before

After

Install yarn

Usaging yarn

use from inside a view

use from controller

TODO

theforeman-bot commented Nov 27, 2017

sharvit Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharvit commented Nov 27, 2017

xprazak2 Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

xprazak2 commented Nov 28, 2017

sharvit commented Nov 28, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharvit Nov 29, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharvit commented Nov 29, 2017

xprazak2 commented Nov 30, 2017

sharvit commented Nov 30, 2017

tstrachota commented Jan 4, 2018

sharvit commented Jan 4, 2018

himdel commented Aug 19, 2018

sharvit commented Aug 20, 2018

mmoll commented Sep 1, 2018

ohadlevy commented Oct 8, 2018

johnpmitsch commented Oct 12, 2018

ekohl commented Oct 13, 2018

johnpmitsch commented Oct 15, 2018

iNecas commented Jan 7, 2019

ares commented Jan 7, 2019

sharvit commented Nov 27, 2017 •

edited

Loading

sharvit Nov 27, 2017 •

edited

Loading

xprazak2 Nov 28, 2017 •

edited

Loading

sharvit Nov 29, 2017 •

edited

Loading