split up base JS into vendor/app #860

tilsammans · 2017-09-07T06:07:08Z

This pattern is quite common for Webpack and it makes a lot of sense to me too.

We split up application.js into a vendor part and an app part. The engine precompiles both. They are meant to be included separately into the html . Vendor won't change nearly as often and so will remain cached for longer.

It's backwards compatible with the current way. I'll update docs etc if you think this is a good idea.

houndci-bot · 2017-09-07T06:07:19Z

app/assets/javascripts/pageflow/base.js

-    return (window.location.href.indexOf('debug=true') >= 0);
-  }
-};
+pageflow.log("Rails Asset Pipeline: <pageflow/base> is deprecated and will be removed from Pageflow. Use separate vendor and app assets.");


Line is too long.

houndci-bot · 2017-09-07T06:07:19Z

app/assets/javascripts/pageflow/app.js

+//= require_tree ./widgets
+//= require ./react
+
+pageflow = {


tilsammans · 2017-09-07T06:15:57Z

The way HTTP/2 streams all assets together makes the enormous asset bundles obsolete IMO.

coveralls · 2017-09-07T06:20:20Z

Coverage decreased (-4.8%) to 94.326% when pulling 5fbe5eb on scrollytelling:vendor-app-javascript into b3d492b on codevise:master.

coveralls · 2017-09-07T06:20:20Z

Coverage decreased (-4.8%) to 94.326% when pulling 5fbe5eb on scrollytelling:vendor-app-javascript into b3d492b on codevise:master.

tf

Makes sense. Still, since Pageflow renders the script tags itself, I'd suggest inserting a script tag for pageflow/vendor.js there. We can then remove vendor related stuff from pageflow/base.js but keep everything else as is. That way pageflow/applications.js files in host apps do not have to change at all. Only the //= require pageflow/base directive will pull in a lot less stuff.

houndci-bot · 2017-09-16T20:17:23Z

lib/pageflow/engine.rb

@@ -88,5 +88,9 @@ class Engine < ::Rails::Engine
        FactoryGirl.definition_file_paths.unshift(Engine.root.join('spec', 'factories'))
      end
    end
+
+    initializer "pageflow.assets.precompile" do |app|
+      app.config.assets.precompile += %w( vendor.js app.js )


Do not use spaces inside percent literal delimiters.

houndci-bot · 2017-09-16T20:17:23Z

lib/pageflow/engine.rb

@@ -88,5 +88,9 @@ class Engine < ::Rails::Engine
        FactoryGirl.definition_file_paths.unshift(Engine.root.join('spec', 'factories'))
      end
    end
+
+    initializer "pageflow.assets.precompile" do |app|


Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2017-09-16T21:05:16Z

lib/pageflow/engine.rb

+         end
+       end
+
+       app.config.assets.precompile << lambda do |path, filename|


Inconsistent indentation detected.

houndci-bot · 2017-09-16T21:05:16Z

lib/pageflow/engine.rb

+                                     pageflow/vendor.js
+                                     video-js.swf vjs.eot vjs.svg vjs.ttf vjs.woff)
+
+       app.config.assets.precompile << lambda do |path, _filename|


Inconsistent indentation detected.

houndci-bot · 2017-09-16T21:05:16Z

lib/pageflow/engine.rb

+                                     pageflow/print_view.css
+                                     pageflow/lt_ie9.js pageflow/lt_ie9.css pageflow/ie9.js pageflow/ie9.css
+                                     pageflow/vendor.js
+                                     video-js.swf vjs.eot vjs.svg vjs.ttf vjs.woff)


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-09-16T21:05:16Z

lib/pageflow/engine.rb

+                                     pageflow/application_with_simulated_media_queries.css
+                                     pageflow/print_view.css
+                                     pageflow/lt_ie9.js pageflow/lt_ie9.css pageflow/ie9.js pageflow/ie9.css
+                                     pageflow/vendor.js


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-09-16T21:05:16Z

lib/pageflow/engine.rb

+      app.config.assets.precompile += %w(pageflow/editor.js pageflow/editor.css
+                                     pageflow/application_with_simulated_media_queries.css
+                                     pageflow/print_view.css
+                                     pageflow/lt_ie9.js pageflow/lt_ie9.css pageflow/ie9.js pageflow/ie9.css


Align the elements of an array literal if they span more than one line.
Line is too long. [108/100]

houndci-bot · 2017-09-16T21:05:16Z

lib/pageflow/engine.rb

+    initializer 'pageflow.assets.precompile' do |app|
+      app.config.assets.precompile += %w(pageflow/editor.js pageflow/editor.css
+                                     pageflow/application_with_simulated_media_queries.css
+                                     pageflow/print_view.css


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-09-16T21:05:16Z

lib/pageflow/engine.rb

+    # provided by the main app.
+    initializer 'pageflow.assets.precompile' do |app|
+      app.config.assets.precompile += %w(pageflow/editor.js pageflow/editor.css
+                                     pageflow/application_with_simulated_media_queries.css


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-09-16T21:05:17Z

lib/pageflow/engine.rb

+         end
+       end
+
+       app.config.assets.precompile << lambda do |path, filename|


Inconsistent indentation detected.

houndci-bot · 2017-09-16T21:05:17Z

lib/pageflow/engine.rb

+                                     pageflow/vendor.js
+                                     video-js.swf vjs.eot vjs.svg vjs.ttf vjs.woff)
+
+       app.config.assets.precompile << lambda do |path, _filename|


Inconsistent indentation detected.

houndci-bot · 2017-09-16T21:05:17Z

lib/pageflow/engine.rb

+                                     pageflow/print_view.css
+                                     pageflow/lt_ie9.js pageflow/lt_ie9.css pageflow/ie9.js pageflow/ie9.css
+                                     pageflow/vendor.js
+                                     video-js.swf vjs.eot vjs.svg vjs.ttf vjs.woff)


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-09-16T21:05:17Z

lib/pageflow/engine.rb

+                                     pageflow/application_with_simulated_media_queries.css
+                                     pageflow/print_view.css
+                                     pageflow/lt_ie9.js pageflow/lt_ie9.css pageflow/ie9.js pageflow/ie9.css
+                                     pageflow/vendor.js


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-09-16T21:05:17Z

lib/pageflow/engine.rb

+      app.config.assets.precompile += %w(pageflow/editor.js pageflow/editor.css
+                                     pageflow/application_with_simulated_media_queries.css
+                                     pageflow/print_view.css
+                                     pageflow/lt_ie9.js pageflow/lt_ie9.css pageflow/ie9.js pageflow/ie9.css


Align the elements of an array literal if they span more than one line.
Line is too long. [108/100]

houndci-bot · 2017-09-16T21:05:17Z

lib/pageflow/engine.rb

+    initializer 'pageflow.assets.precompile' do |app|
+      app.config.assets.precompile += %w(pageflow/editor.js pageflow/editor.css
+                                     pageflow/application_with_simulated_media_queries.css
+                                     pageflow/print_view.css


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-09-16T21:05:17Z

lib/pageflow/engine.rb

+    # provided by the main app.
+    initializer 'pageflow.assets.precompile' do |app|
+      app.config.assets.precompile += %w(pageflow/editor.js pageflow/editor.css
+                                     pageflow/application_with_simulated_media_queries.css


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-09-16T21:05:17Z

lib/pageflow/engine.rb

+         end
+       end
+
+       app.config.assets.precompile << lambda do |path, filename|


Inconsistent indentation detected.

houndci-bot · 2017-09-16T21:05:17Z

lib/pageflow/engine.rb

+                                     pageflow/vendor.js
+                                     video-js.swf vjs.eot vjs.svg vjs.ttf vjs.woff)
+
+       app.config.assets.precompile << lambda do |path, _filename|


Inconsistent indentation detected.

houndci-bot · 2017-09-16T21:05:18Z

lib/pageflow/engine.rb

+                                     pageflow/print_view.css
+                                     pageflow/lt_ie9.js pageflow/lt_ie9.css pageflow/ie9.js pageflow/ie9.css
+                                     pageflow/vendor.js
+                                     video-js.swf vjs.eot vjs.svg vjs.ttf vjs.woff)


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-09-16T21:05:18Z

lib/pageflow/engine.rb

+                                     pageflow/application_with_simulated_media_queries.css
+                                     pageflow/print_view.css
+                                     pageflow/lt_ie9.js pageflow/lt_ie9.css pageflow/ie9.js pageflow/ie9.css
+                                     pageflow/vendor.js


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-09-16T21:05:18Z

lib/pageflow/engine.rb

+      app.config.assets.precompile += %w(pageflow/editor.js pageflow/editor.css
+                                     pageflow/application_with_simulated_media_queries.css
+                                     pageflow/print_view.css
+                                     pageflow/lt_ie9.js pageflow/lt_ie9.css pageflow/ie9.js pageflow/ie9.css


Align the elements of an array literal if they span more than one line.
Line is too long. [108/100]

houndci-bot · 2017-09-16T21:05:18Z

lib/pageflow/engine.rb

+    initializer 'pageflow.assets.precompile' do |app|
+      app.config.assets.precompile += %w(pageflow/editor.js pageflow/editor.css
+                                     pageflow/application_with_simulated_media_queries.css
+                                     pageflow/print_view.css


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-09-16T21:05:18Z

lib/pageflow/engine.rb

+    # provided by the main app.
+    initializer 'pageflow.assets.precompile' do |app|
+      app.config.assets.precompile += %w(pageflow/editor.js pageflow/editor.css
+                                     pageflow/application_with_simulated_media_queries.css


Align the elements of an array literal if they span more than one line.

tilsammans · 2017-09-16T21:05:26Z

@tf changes done & rebased

coveralls · 2017-09-16T21:15:30Z

Coverage remained the same at 99.169% when pulling ffd21ea on scrollytelling:vendor-app-javascript into a8a53e5 on codevise:master.

coveralls · 2017-09-16T21:15:31Z

Coverage remained the same at 99.169% when pulling ffd21ea on scrollytelling:vendor-app-javascript into a8a53e5 on codevise:master.

tf

Specs are failing since Teaspoon does not load the new vendor.js file.

tf · 2017-10-09T15:29:32Z

lib/pageflow/engine.rb

+         filename.start_with?(Engine.root.join('app/assets').to_s) &&
+           !['.js', '.css', ''].include?(File.extname(path))
+       end
+    end


What difference does wrapping the code in an initializer block make here? Can we fix indentation here?

Just following the documented API for engine asset precompilation. Pretty sure this will handle a tweaked asset pipeline better.

Indentation fixed.

tf · 2017-10-09T15:30:03Z

app/assets/javascripts/pageflow/vendor.js

@@ -0,0 +1,16 @@
+// vendor stylesheet for Pageflow.


"stylesheet" is a typo.

houndci-bot · 2017-10-14T00:58:32Z

lib/pageflow/engine.rb

+      end
+
+      app.config.assets.precompile << lambda do |path, filename|
+        filename.start_with?(Engine.root.join('app/assets').to_s) && !['.js', '.css', ''].include?(File.extname(path))


Line is too long. [118/100]

tilsammans · 2017-10-14T00:59:04Z

@tf

Specs are failing since Teaspoon does not load the new vendor.js file.

I am not too familiar with Teaspoon, how do I fix that?

Other changes are done.

tf · 2017-10-16T09:12:05Z

Tried to push a commit to the PR, but maybe the option is not set. Here's the diff, that makes teaspoon pass: https://github.com/scrollytelling/pageflow/compare/vendor-app-javascript...tf:vendor-app-javascript

tilsammans · 2017-10-16T11:34:23Z

I've merged in your commits.

tf · 2017-10-17T07:11:24Z

Travis appears to be stuck somehow...

These will be compiled separately. Since they change less often, the idea is that these assets will remain in cache for longer.

tilsammans · 2017-10-20T17:58:44Z

@tf squashed all the commits (including yours).

tf · 2017-10-20T18:35:48Z

Thanks. I'd totally merge, but I want the green check mark on the PR first ;) I don't know why Travis stopped picking up changes, but worst case we'll have to create a fresh PR from the same branch.

tilsammans · 2017-10-20T18:53:22Z

I'll create a new PR tomorrow if it doesn't go through today.

tilsammans requested a review from tf September 7, 2017 06:07

tilsammans self-assigned this Sep 7, 2017

houndci-bot reviewed Sep 7, 2017

View reviewed changes

tilsammans added the improvement label Sep 7, 2017

tf requested changes Sep 13, 2017

View reviewed changes

houndci-bot reviewed Sep 16, 2017

View reviewed changes

tf requested changes Oct 9, 2017

View reviewed changes

houndci-bot reviewed Oct 14, 2017

View reviewed changes

tf approved these changes Oct 16, 2017

View reviewed changes

Split vendored code from our own

c34ded3

These will be compiled separately. Since they change less often, the idea is that these assets will remain in cache for longer.

tilsammans closed this Oct 21, 2017

tilsammans mentioned this pull request Oct 21, 2017

Split vendored code from our own #885

Merged

split up base JS into vendor/app #860

split up base JS into vendor/app #860

Conversation

tilsammans commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tilsammans commented Sep 7, 2017

coveralls commented Sep 7, 2017

coveralls commented Sep 7, 2017

tf left a 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

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

tilsammans commented Sep 16, 2017

coveralls commented Sep 16, 2017

coveralls commented Sep 16, 2017

tf left a 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

tilsammans commented Oct 14, 2017

tf commented Oct 16, 2017

tilsammans commented Oct 16, 2017

tf commented Oct 17, 2017

tilsammans commented Oct 20, 2017

tf commented Oct 20, 2017

tilsammans commented Oct 20, 2017