Feature/merge server client webpack #398

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt, line 14 [r1] (raw file):
@justin808 I dove more into the lib code and it seems it may be more complicated than we hoped. Right now the code is tied to the name's server for a bundle name and there are lines that are strictly tied to npm run build:server and npm run build:client.

response = `pgrep -fl 'bin/webpack\s(\\-w|\\-\\-watch)\s\\-\\-config\s.*#{type}.*\\.js'`

and

build_output = `cd client && npm run build:#{type}`

Now I think we have a few options.

Always require that the server bundle is also used in the client. Now there could be multiple generated bundles, but one of the client bundles would have to be used for the server as well.
Not sure if this will work but having npm run build:server and npm run build:client but they just pass variables to the same config to spit out different files. At that point it may make more sense to just go with 2 configs though.
we add a dynamic element that allows people to add on npm commands to do whatever like server render a separate config if needed.

I'm personally in favor of just going for a single config since most people will just be putting the entire bundle in their header which is awesome with turbolinks. And if you need to optmize, then you can still do vendor chunks as needed which would still play nice with all of this.

Comments from Reviewable

jbhatab · 2016-04-24T10:03:49Z

lib/generators/USAGE, line 3 [r1] (raw file):
Done.

Comments from Reviewable

jbhatab · 2016-04-30T04:03:30Z

Just changed the webpack compilation checks for the tests. Now it just checks to see if any stale files exist and then runs a new config variable we added if they do. we default to 'npm run build'.

coveralls · 2016-04-30T04:21:46Z

Coverage increased (+0.4%) to 85.693% when pulling aa3d80f on feature/merge-server-client-webpack into 9995619 on master.

justin808 · 2016-05-02T21:09:42Z

Reviewed 1 of 39 files at r1, 9 of 12 files at r2, 6 of 11 files at r3.
Review status: 40 of 48 files reviewed at latest revision, 8 unresolved discussions.

lib/react_on_rails.rb, line 17 [r3] (raw file):
Where did this go?

lib/generators/react_on_rails/templates/base/base/package.json.tt, line 12 [r1] (raw file):
why remove these?

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt, line 14 [r1] (raw file):
what if we just check for a "webpack -w" ?

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 11 [r1] (raw file):
Are you not finished with this one?

We should pass _props and _railsContext and use neither, as they will be used for store initialization.

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 12 [r1] (raw file):
Not done yet

lib/react_on_rails/test_helper.rb, line 57 [r3] (raw file):
Why are we removing this? Let's keep it.

lib/react_on_rails/test_helper/ensure_assets_compiled.rb, line 49 [r3] (raw file):
Let's not remove all of this

Comments from Reviewable

justin808 · 2016-05-02T21:10:10Z

Got some questions. Also Please see https://github.com/shakacode/react_on_rails/blob/master/docs%2Fcontributor-info%2Fcontributing.md. Notably, we need a changelog entry.

Review status: 40 of 48 files reviewed at latest revision, 8 unresolved discussions.

Comments from Reviewable

justin808 · 2016-05-06T08:38:04Z

@jbhatab Is this almost ready?

robwise · 2016-05-07T01:31:40Z

Reviewed 1 of 39 files at r1, 4 of 12 files at r2, 7 of 11 files at r3.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

lib/react_on_rails.rb, line 17 [r3] (raw file):
We deleted it, it's super super brittle/unmaintainable and was not working anyway. Simply checking if the assets are up to date or not should be sufficient. There is only the unlikely case where a user changes something inside of the client code and is able to start the tests so fast that they kick off the ensure assets compiled process before the webpack process has finished compiling, in which case it just re-compiles so not that painful.

lib/generators/react_on_rails/templates/base/base/package.json.tt, line 12 [r1] (raw file):
it isn't DRY and therefore is extremely brittle

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt, line 14 [r1] (raw file):
we got rid of the process checker, this is a moot point now, right @jbhatab ?

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 11 [r1] (raw file):
see below comment

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 12 [r1] (raw file):
using the shared store is a very advanced, niche feature. I don't think we should generate it in a simple hello world example, as it's just going to confuse people. We should keep it as vanilla as possible and leave the advanced redux store feature for the API docs and possibly a recipe section of the docs.

lib/react_on_rails/test_helper.rb, line 57 [r3] (raw file):
See previous comment

lib/react_on_rails/test_helper/ensure_assets_compiled.rb, line 49 [r3] (raw file):
this code is extremely brittle and not surprisingly was broken. Now that it's fixed, I highly suggest keeping it this way. It's much easier to read and works correctly now

Comments from Reviewable

robwise · 2016-05-07T02:23:39Z

Reviewed 26 of 39 files at r1, 10 of 12 files at r2, 8 of 11 files at r3.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

lib/generators/react_on_rails/templates/base/base/app/views/hello_world/index.html.erb.tt, line 2 [r1] (raw file):
why do we want to explicitly override the default the user sets in the config here? Isn't that a little assuming? It's also one additional thing they have to understand when first getting used to our API

Comments from Reviewable

jbhatab · 2016-05-07T19:15:12Z

lib/react_on_rails.rb, line 17 [r3] (raw file):
Yeah, that was the general logic. It was too fragile and the scenarios where it even mattered we're very rare.

Comments from Reviewable

jbhatab · 2016-05-07T19:16:33Z

lib/generators/react_on_rails/templates/base/base/package.json.tt, line 12 [r1] (raw file):
Even if we do keep these, we would condense them down to build and build:dev vs having all of them, but I think we should keep all of the npm commands that are related to the client directory in the package.json client directory. This adds a layer of magic that may confuse new people into thinking it's necessary to use react_on_rails.

Comments from Reviewable

jbhatab · 2016-05-07T19:17:36Z

lib/generators/react_on_rails/templates/base/base/app/views/hello_world/index.html.erb.tt, line 2 [r1] (raw file):
We can change this later if people complain. I think it's pretty minor.

Comments from Reviewable

jbhatab · 2016-05-07T19:18:41Z

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt, line 14 [r1] (raw file):
Yup, we removed the process checker. It was very fragile and already brought up a bug where the core functionality of checking to see if the webpack file was stale broke. Also if people change their npm commands then it becomes useless.

Comments from Reviewable

jbhatab · 2016-05-07T19:19:51Z

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 12 [r1] (raw file):
@justin808 I want to add this in a separate PR or in the component generator PR. This PR is already getting a little big and we should merge this in first. I agree that it's a cool feature to show off.

Comments from Reviewable

jbhatab · 2016-05-07T19:21:30Z

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 22 [r1] (raw file):
show's a line ending for me.

Comments from Reviewable

coveralls · 2016-05-08T15:55:36Z

Coverage increased (+0.09%) to 82.568% when pulling 5418c3b on feature/merge-server-client-webpack into 9491179 on master.

coveralls · 2016-05-08T15:58:25Z

Coverage increased (+0.09%) to 82.568% when pulling 5418c3b on feature/merge-server-client-webpack into 9491179 on master.

justin808 · 2016-05-09T06:08:20Z

lib/generators/USAGE


 * Redux

    Passing the --redux generator option causes the generated Hello World example
    to integrate the Redux state container framework. The necessary node modules
    will be automatically included for you.

-    The generator uses the organizational `paradigm of "bundles"`. These are like


Which file did this go to? or just removed, @jbhatab

coveralls · 2016-05-09T06:20:39Z

Coverage increased (+0.09%) to 82.568% when pulling a26f399 on feature/merge-server-client-webpack into 9491179 on master.

coveralls · 2016-05-09T06:21:38Z

Coverage increased (+0.09%) to 82.568% when pulling a26f399 on feature/merge-server-client-webpack into 9491179 on master.

Objective of these changes is to make the generator simpler, partly by simplfying the default behavior of ReactOnRails. * combined webpack files into one webpack config, fixed corresponding tests and code. cleaned up some documentation. * removed jquery, fixed some linting. removed lodash completely * changed asset compliation checks * moved files from server rendering to base for templates * changed server-bundle to webpack-bundle * added prodcution build command and test build command. changed configuration to test build command to be more explicit. changed around some readmes * changed build:dev to build:development

justin808 · 2016-05-10T06:47:13Z

docs/api/ruby-api-hot-reload-view-helpers.md

@@ -13,8 +13,7 @@ static vs. hot is picked based on whether `ENV["REACT_ON_RAILS_ENV"] == "HOT"`

 <!-- These do not use turbolinks, so no data-turbolinks-track -->
 <!-- This is to load the hot assets. -->
- <%= env_javascript_include_tag(hot: ['http://localhost:3500/vendor-bundle.js',
-                                      'http://localhost:3500/app-bundle.js']) %>
+ <%= env_javascript_include_tag(hot: ['http://localhost:3500/webpack-bundle.js') %>


@jbhatab no reason to change this doc file. the point was to show that it can take multiple files. It doesn't need to correspond to the generated file, and you missed the closing square bracket.

coveralls · 2016-05-10T09:00:21Z

Coverage increased (+0.05%) to 82.52% when pulling 540aa79 on feature/merge-server-client-webpack into 56c0c8d on master.

justin808 · 2016-05-10T09:03:26Z

@jbhatab @robwise Please review my last commit. I'm going to create a beta release and test on the tutorial app.

justin808 · 2016-05-10T12:05:31Z

@samnang, @jbhatab @robwise can you review a9f1f38

This is deployed live at http://www.reactrails.com.

Using: shakacode/react-webpack-rails-tutorial#287

coveralls · 2016-05-10T12:31:02Z

Coverage increased (+0.07%) to 82.544% when pulling f80eb14 on feature/merge-server-client-webpack into 56c0c8d on master.

coveralls · 2016-05-10T12:33:08Z

Coverage increased (+0.07%) to 82.544% when pulling f80eb14 on feature/merge-server-client-webpack into 56c0c8d on master.

coveralls · 2016-05-10T12:39:13Z

Coverage increased (+0.07%) to 82.544% when pulling f80eb14 on feature/merge-server-client-webpack into 56c0c8d on master.

jbhatab · 2016-05-10T17:41:22Z

lib/generators/USAGE, line 3 [r10] (raw file):

Previously, justin808 (Justin Gordon) wrote…

this is not a markdown file

needed a CR

CR?

Comments from Reviewable

robwise · 2016-05-10T17:56:28Z

Reviewed 2 of 3 files at r9, 10 of 15 files at r11, 6 of 10 files at r12, 4 of 4 files at r13.
Review status: all files reviewed at latest revision, 14 unresolved discussions.

lib/generators/USAGE, line 3 [r10] (raw file):

Previously, jbhatab (Blaine Hatab) wrote…

CR?

carriage return

lib/react_on_rails/test_helper/ensure_assets_compiled.rb, line 48 [r13] (raw file):

React on Rails will ensure your JavaScript generated files are up to date, using your
/client level package.json `build:test` command.

what's with the slash here, we want the line return preserved since this is a console message

lib/tasks/assets.rake, line 113 [r13] (raw file):

# puts "Enhancing assets:precompile with react_on_rails:assets:compile_environment"
# Rake::Task["assets:precompile"]
#   .clear_prerequisites

@justin808 did we ever figure out what clear_prerequisites does?

spec/dummy/config/initializers/react_on_rails.rb, line 23 [r13] (raw file):

  # If you are using the same file for client and server rendering, having this set probably does
  # not affect performance.
  config.server_bundle_js_file = "server-bundle.js"

shouldn't this be set to webpack-bundle.js by default, since by default we don't generate separate bundles?

spec/react_on_rails/test_helper/ensure_assets_compiled_spec.rb, line 16 [r10] (raw file):

Previously, justin808 (Justin Gordon) wrote…

where is this defined?

lib/generators/react_on_rails/templates/base/base/client/.DS_Store, line 0 [r7] (raw file):
this is still in the commit, pls delete

Comments from Reviewable

robwise · 2016-05-10T18:00:05Z

Reviewed 1 of 7 files at r4, 5 of 8 files at r5, 5 of 16 files at r6, 2 of 5 files at r7.
Review status: all files reviewed at latest revision, 15 unresolved discussions.

lib/generators/react_on_rails/templates/base/base/.DS_Store, line 0 [r13] (raw file):
.DS_Store

Comments from Reviewable

robwise · 2016-05-10T18:11:28Z

Review status: all files reviewed at latest revision, 13 unresolved discussions.

lib/generators/react_on_rails/base_generator.rb, line 97 [r6] (raw file):

Previously, justin808 (Justin Gordon) wrote…

how-to is appropriate.

let's do that, I think these files will definitely confuse newbies

Comments from Reviewable

justin808 · 2016-05-11T08:12:39Z

Ready? I think so.

Previously, coveralls wrote…

Coverage increased (+0.07%) to 82.544% when pulling f80eb14 on feature/merge-server-client-webpack into 56c0c8d on master.

Review status: all files reviewed at latest revision, 10 unresolved discussions.

lib/generators/react_on_rails/base_generator.rb, line 97 [r6] (raw file):

Previously, justin808 (Justin Gordon) wrote…

how-to is appropriate.

we're not going copy either.

lib/generators/react_on_rails/templates/base/base/.DS_Store, line [r13] (raw file):

Previously, robwise (Rob Wise) wrote…

.DS_Store

removed

lib/generators/react_on_rails/templates/redux/base/client/app/bundles/HelloWorld/startup/HelloWorldApp.jsx.tt, line 12 [r1] (raw file):

Previously, jbhatab (Blaine Hatab) wrote…

It failed CI for some reason. I can add an eslint ignore to this file but that feels off. We could disable it globally. Does it have file targeting in the config?

Probably had to use a underscore. I changed this and added commentsjj

lib/react_on_rails/test_helper/ensure_assets_compiled.rb, line 48 [r13] (raw file):

Previously, robwise (Rob Wise) wrote…

what's with the slash here, we want the line return preserved since this is a console message

to mean this is the top level client directory

lib/tasks/assets.rake, line 113 [r13] (raw file):

Previously, robwise (Rob Wise) wrote…

@justin808 did we ever figure out what clear_prerequisites does?

yes -- removes the stuff to do before the task

I documented in this file on why it's needed above.

spec/dummy/config/initializers/react_on_rails.rb, line 23 [r13] (raw file):

Previously, robwise (Rob Wise) wrote…

shouldn't this be set to webpack-bundle.js by default, since by default we don't generate separate bundles?

the spec dummy example uses separate files

we didn't change that (no diff mark)

Comments from Reviewable

Misc fine tuning * Update travis to ruby 2.3.1 * Fixed doc in config file * Update the assets.rake inclusion, as it cannot read the configuration when defining tasks. * Add require for Addressable

jbhatab force-pushed the feature/merge-server-client-webpack branch 3 times, most recently from 63074b5 to 8401c9d Compare April 24, 2016 04:04

jbhatab force-pushed the feature/merge-server-client-webpack branch from 0ee40ba to aa3d80f Compare April 30, 2016 04:01

justin808 added the ready to merge label May 6, 2016

robwise mentioned this pull request May 7, 2016

Always builds bundle in tests #394

Closed

jbhatab force-pushed the feature/merge-server-client-webpack branch from 8397068 to 7dcc845 Compare May 8, 2016 15:40

justin808 reviewed May 9, 2016
View reviewed changes

justin808 reviewed May 10, 2016
View reviewed changes

justin808 force-pushed the feature/merge-server-client-webpack branch 2 times, most recently from c19304e to 540aa79 Compare May 10, 2016 08:52

justin808 force-pushed the feature/merge-server-client-webpack branch 4 times, most recently from 08eaadc to a9f1f38 Compare May 10, 2016 12:03

justin808 force-pushed the feature/merge-server-client-webpack branch from a11f7f7 to 89202e1 Compare May 10, 2016 12:12

Mainly updated docs

6286cdd

Misc fine tuning * Update travis to ruby 2.3.1 * Fixed doc in config file * Update the assets.rake inclusion, as it cannot read the configuration when defining tasks. * Add require for Addressable

justin808 force-pushed the feature/merge-server-client-webpack branch from ecb3325 to 6286cdd Compare May 11, 2016 09:01

justin808 merged commit 0118cea into master May 11, 2016

justin808 deleted the feature/merge-server-client-webpack branch May 11, 2016 09:01

Feature/merge server client webpack #398

Feature/merge server client webpack #398

Conversation

jbhatab commented Apr 23, 2016

justin808 commented Apr 24, 2016

jbhatab commented Apr 24, 2016

jbhatab commented Apr 24, 2016

jbhatab commented Apr 24, 2016

coveralls commented Apr 24, 2016

jbhatab commented Apr 24, 2016

jbhatab commented Apr 24, 2016

jbhatab commented Apr 24, 2016

jbhatab commented Apr 30, 2016

coveralls commented Apr 30, 2016

justin808 commented May 2, 2016

justin808 commented May 2, 2016

justin808 commented May 6, 2016

robwise commented May 7, 2016

robwise commented May 7, 2016

jbhatab commented May 7, 2016

jbhatab commented May 7, 2016

jbhatab commented May 7, 2016

jbhatab commented May 7, 2016

jbhatab commented May 7, 2016

jbhatab commented May 7, 2016

coveralls commented May 8, 2016

coveralls commented May 8, 2016

justin808 May 9, 2016

Choose a reason for hiding this comment

coveralls commented May 9, 2016

coveralls commented May 9, 2016

justin808 May 10, 2016

Choose a reason for hiding this comment

coveralls commented May 10, 2016

justin808 commented May 10, 2016

justin808 commented May 10, 2016

coveralls commented May 10, 2016

coveralls commented May 10, 2016

coveralls commented May 10, 2016

jbhatab commented May 10, 2016

robwise commented May 10, 2016

robwise commented May 10, 2016

robwise commented May 10, 2016

justin808 commented May 11, 2016