Support for multiple server file bundles

[lucke84]

Adds support for multiple server bundle js files, as a follow-up of this conversation: #317

---

This change is

[lucke84]

Please take a look at the PR: there's still some testing, clean-up and documentation work to do, but we would like to have a first round of feedback on:

• the approach, in general (including the pool-resetting triggering mechanism for development mode)
• the testing part (what would you add/change)
• the defaulting assumption (the first entry in the array on bundle js file is the default one, when not specified)
• miscellaneous (we tried to stick with the project's best practices and conventions, we might have missed something)

Looking forward to seeing this bit of functionality supported, we (and probably a lot of others) will benefit from this mechanism.

[justin808]

Looks like a great start.

Let me know if you have questions.

---

Reviewed 4 of 8 files at r1, 22 of 22 files at r3.
Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks broke.

---

README.md, line 225 [r3] (raw file):
Why did you change this?

---

app/helpers/react_on_rails_helper.rb, line 87 [r3] (raw file):

#   server_bundle_js_file: the name of the server bundle js file you want to use for rendering the component server side.
# If this is not specified, the default file specified in your config/initializers/react_on_rails.rb is used.

---

lib/generators/react_on_rails/base_generator.rb, line 150 [r3] (raw file):
server.jsx is too generic

---

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt, line 18 [r3] (raw file):
I'm not 100% sure we want to change the config for this. It's an easy migration, but I'm not sure it's common enough to have multiple files.

---

lib/react_on_rails/configuration.rb, line 13 [r3] (raw file):

files +=  @configuration.server_bundle_js_files

That's what I normally do, but either would work. Use parens if you use a method call.

---

lib/react_on_rails/configuration.rb, line 43 [r3] (raw file):

def normalize_server_bundle_js_files
 @configuration.server_bundle_js_files = @configuration.server_bundle_js_files.map do |server_bundle_js_file|
      if server_bundle_js_file.include?(File::SEPARATOR)
        puts "[DEPRECATION] ReactOnRails: remove path from server_bundle_js_files in configuration. "\
          "All generated files must go in #{@configuration.generated_assets_dir}"
        server_bundle_js_file = File.basename(server_bundle_js_file)
      else
        server_bundle_js_file 
      end
    end
end

---

lib/react_on_rails/server_rendering_pool.rb, line 16 [r3] (raw file):

  @js_context_pools = 
     ReactOnRails.configuration.server_bundle_js_files.each_with_object({}) do |server_bundle_js_file, hash|
        hash[server_bundle_js_file] = ConnectionPool.new(options) do
          create_js_context(server_bundle_js_file)
        end
      end

---

lib/react_on_rails/server_rendering_pool.rb, line 74 [r3] (raw file):
let's make an accessor method

js_context_pool_for_file

---

lib/react_on_rails/server_rendering_pool.rb, line 95 [r3] (raw file):
this is a valid case that need to be checked. I.e., the code supports evaluating a piece of arbitrary JS on the server side.

Normally, you'd have your default server_js_file used, but there is a case where you don't have any server js file.

---

lib/react_on_rails/utils.rb, line 22 [r3] (raw file):
This makes sense. We need to be sure we document this well.

---

spec/dummy/client/webpack.server.js, line 7 [r3] (raw file):
See this for setting up the entry points:

https://github.com/shakacode/react-webpack-rails-tutorial/blob/master/client/webpack.client.base.config.js#L17

  entry: {

    // See use of 'vendor' in the CommonsChunkPlugin inclusion below.
    vendor: [
      'babel-polyfill',
      'es5-shim/es5-shim',
      'es5-shim/es5-sham',
      'jquery',
    ],

    // This will contain the app entry points defined by webpack.hot.config and webpack.rails.config
    app: [
      './app/bundles/comments/startup/clientRegistration',
    ],
  },

---

spec/dummy/client/webpack.server.js, line 11 [r3] (raw file):
I bet this is why you renamed serverRegistration. Please change it back.

---

spec/dummy/client/webpack.server.rails.build.config.js, line 15 [r3] (raw file):
This looks correct. Maybe webpack.server was a file that should have been removed. It seems replaced by this file.

---

spec/dummy/client/app/startup/alternative-server.jsx, line 1 [r3] (raw file):
For the example, why not just include only the one component that has a different server bundle?

Or else we really need to refactor this.

---

Comments from the review on Reviewable.io

[justin808]

@jopdeklein @lucke84 We need to make sure the docs provide a really solid use case for why we need different bundles. I would think we maybe able to accomplish the same end result by passing in a prop from rails and having a few conditionals in the code.

In the case of client side bundles, there are clear reasons on why you'd separate the bundles:

1. vendor-bundle.js can change far less than your app code, so client browsers will have this file cached.
2. app-bundle.js can change frequently.
3. Some mobile pages may only want to load a bit of the js.

I don't see any of these applying to the server side.

Also see #287. Could that be a simpler solution.

Basically, before we add this feature, we need to be 100% sure that the extra complexity of this is useful.

Furthermore, we'll eventually move to using #183 for server rendering.

CC: @alexfedoseev @jbhatab @robwise @thewoolleyman

[lucke84]

#287 is indeed something useful but if you need separate bundles in a more consistent way, it'll end up having the developers using this gem to hack a lot in order to achieve something that can work out of the box. At the same time, I agree on the fact this feature would introduce some complexity which has to be justified by a real-life, common need. Later on today we'll add a more structured comment to explain a proper use case. For now, I'm just about to update the PR with a couple changes to address part of your feedback (see specific comments on each item).

---

Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks broke.

---

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt, line 18 [r3] (raw file):
We were actually wondering what's the best way to deal with this. Maybe it's a bit old-school, but I like the consistency of a field's type, that's why I changed this to be always an array. Plus, at the moment it's probably not common at all because the framework doesn't support the functionality yet but, if well documented, I think people will start using it more and more often.

Anyhow, what would you recommend?

• To have a multi-type field (Ruby-style), which can be either string or array
• always the array with the assumption that the first file is the default
• an extra field to specify when using only one server bundle file

---

lib/react_on_rails/configuration.rb, line 13 [r3] (raw file):
concat is better memory-wise, but again in this context += is not a problem.

---

lib/react_on_rails/configuration.rb, line 43 [r3] (raw file):
Happy to extract this, as long as it is a private method :) Plus, I think it should be a class method, shouldn't it?

---

lib/react_on_rails/server_rendering_pool.rb, line 16 [r3] (raw file):
each_with_object is handy but turns out it is not very efficient memory-wise. Since it doesn't get triggered that often (especially in prod) I'll change that.

---

lib/react_on_rails/server_rendering_pool.rb, line 95 [r3] (raw file):
Can you give an example of a situation like that? I think that being in the SSR context there should always be a bundle js file.

---

Comments from the review on Reviewable.io

[jopdeklein]

Review status: 20 of 24 files reviewed at latest revision, 14 unresolved discussions.

---

README.md, line 225 [r3] (raw file):
Indeed as you mention later we did this to output server-bundle.js and server-alternative-bundle.js to avoid too many changes throughout the code. I think we can fix it in the webpack.config without changing anything else, I'll give it a shot.

---

spec/dummy/client/webpack.server.js, line 7 [r3] (raw file):
OK I see your point, but I am unsure where the outcome of this is used - just for development / hot version of the server rendering? I can set it up similar like we've done for the build but we'll need to find a way to load the correct one:

entry: {
    server: ['babel-polyfill', './app/startup/server'],
    'alternative-server': ['babel-polyfill', './app/startup/alternative-server']
}

Either way we have to discuss the need for different bundles, we'll try to sketch our scenario and we can debate whether our approach is the simplest and most useful, perhaps there are simpler paths to what we want to accomplish.

---

Comments from the review on Reviewable.io

[justin808]

@jopdeklein Think about one big bundle and injecting a little JS code either before or after we render the components, create the stores.

Per #287...

[justin808]

Given our discussion for #287, should we keep this PR open now? We can close and always re-open later if needed. @jopdeklein

[justin808]

@jopdeklein @lucke84 Please see #345. I plan to merge later today. I think this should meet your needs.

[justin808]

@lucke84 @jopdeklein Can we close this one?

[jopdeklein]

@justin808 we're currently using our branch in production in order to fix a critical issue we were facing. We are not looking to deviate too far from your track, so I'll first try whether we can make it work with your proposal of a separate server bundle with a top level switch. If this works it should indeed be simpler than our bundle switching approach.

[justin808]

@jopdeklein you probably don't mean a separate bundle, I'm guessing. You should be able to require whichever file you need dynamically. Use require and not import.

[jopdeklein]

@justin808 You're right, I meant we have a separate server, client-responsive and client-mobile bundles. The server bundle requires the correct components for rendering on the server, while the client bundles are specialized for the corresponding device (basically the RESS approach).

I implemented your suggestions yesterday and it seems to work fine, and @lucke84 and I agree this is a simpler approach - feel free to close this PR.