Providing rails_context always to generator functions (store and component)

[justin808]

Background:

As of v4, we provide the location as a second param to component generator functions after props, and ONLY on the server rendering. This is not in the current API for store generator functions.

Use Case

Needing the current url path for server rendering

Suppose you want to display a nav bar with the current navigation link highlighted by the URL. When you server render the code, you will need to know the current URL/path if that is what you want your logic to be based on. This could be added to props, or ReactOnRails can add this automatically as another param to the generator function (or maybe an additional object, where we'll consider other additional bits of system info from Rails, like maybe the locale, later).

Needing the I18n.locale

Suppose you want to server render your react components with a the current Rails locale. We need to pass the I18n.locale to the view rendering.

Details

We'll do a breaking change of changing the 2nd parameter from a string to an object, but this should affect very few users, as we only passed this for server rendering.

The new api will be to pass 2 params when generating React components or hydrating redux stores:

So if you register MyAppComponent, it will get called like:

reactComponent = MyAppComponent(props, railsContext);

and for a store:

reduxStore = MyReduxStore(props, railsContext);

The railsContext may have:

     {
        # URL settings
        href: request.original_url,
        location: "#{uri.path}#{uri.query.present? ? "?#{uri.query}": ""}",
        scheme: uri.scheme, # http
        host: uri.host, # foo.com
        pathname: uri.path, # /posts
        search: uri.query, # id=30&limit=5

        # Locale settings
        i18nLocale: I18n.locale,
        i18nDefaultLocale: I18n.default_locale,
        httpAcceptLanguage: request.env["HTTP_ACCEPT_LANGUAGE"]
      }

Additionally, you can customize the values.

Set the class for the rendering_extension

  config.rendering_extension = RenderingExtension

Implement it like this:

module RenderingExtension

  # Return a Hash that contains custom values from the view context that will get passed to
  # all calls to react_component and redux_store for rendering
  def self.custom_context(view_context)
     {
       somethingUseful: view_context.session[:something_useful]
     }
  end
end

This would put a prop of somethingUseful in the railsContext passed to all react_component and redux_store calls.

Since you can't access the rails session from JavaScript (or other values available in the view rendering context), this might useful.

---

This change is

[justin808]

My gut tells me to make extra params an object, and leave the first param as the props object, for the JS code. Otherwise, people would have to update their JS app components. They still might have to if they were using the location 2nd param, but that's unlikely to be many people.

[alex35mil]

Maybe 2nd param can be an object? so we have can have { fullUrl: , path: , query: }

👍

Should ReactOnRails be providing this? Certainly this could be added to props. But since this is universal, might make sense to have ReactOnRails do this.

It's needed for react-router on server, so we shouldn't rely on props from user here.

Is the way that I added a store/reducer etc. correct for adding the location info?

Seems fine.

How does this fit with with using redux-react-router? And our sort of app, where we use redux-react-router after the top level URL.

If redux-react-router is used, I guess we should inject location info into its store. Looks like it might be done by just putting it in initial state (link), but I haven't looked much into it, so not sure is this correct way.

[justin808]

@alexfedoseev Given that file with this, how do we inject the current location?

/**
 * This action type will be dispatched when your history
 * receives a location change.
 */
export const LOCATION_CHANGE = '@@router/LOCATION_CHANGE'

const initialState = {
  locationBeforeTransitions: null
}

/**
 * This reducer will update the state with the most recent location history
 * has transitioned to. This may not be in sync with the router, particularly
 * if you have asynchronously-loaded routes, so reading from and relying on
 * this state it is discouraged.
 */
export function routerReducer(state = initialState, { type, payload }) {
  if (type === LOCATION_CHANGE) {
    return { ...state, locationBeforeTransitions: payload }
  }

  return state
}

[justin808]

maybe I return for the extra values:

{ location: { href, pathname, hash, search } }

[jbhatab]

@justin808 I like it as an object as the second variable with all of the location data. Not sure on a string of the full url vs the object. I feel like people are used to parsing urls but generally they get it from window that parses it kinda for them, so I could see it being nice broken up like that.

I'd say object is fine for now.

@alexfedoseev what do you think about string vs object?

[justin808]

Debating on whether or not to pass the location parts, as this library is very popular: https://www.npmjs.com/package/url-parse (550k downloads the past month).

  def rails_context
    uri = URI.parse(request.original_url)
    # uri = URI("http://foo.com/posts?id=30&limit=5#time=1305298413")

    result = {
      # URL settings
      location: request.original_url,
      scheme: uri.scheme, # http
      host: url.host, # foo.com
      pathname: uri.path, # /posts
      search: uri.query, # id=30&limit=5
      hash: uri.fragment, # "time=1305298413"

      # Locale settings
      i18nLocale: I18n.locale,
      i18nDefaultLocale: I18n.default_locale,
      httpAcceptLanguage: request.env['HTTP_ACCEPT_LANGUAGE']
    }

    if ReactOnRails.configuration.rendering_extension
      custom_context = ReactOnRails.configuration.rendering_extension.custom_context(self)
      if custom_context
        result.merge!(custom_context)
      end
    end
    result
  end

This can set the locale quite nicely: https://github.com/iain/http_accept_language.

I'm considering allowing a RenderingExtension like this:

Set the class for the rendering_extension

  config.rendering_extension = RenderingExtension

Implement it like this:

module RenderingExtension

  # Return a Hash that contains custom values from the view context that will get passed to
  # all calls to react_component and redux_store for rendering
  def self.custom_context(view_context)
     {
       somethingUseful: view_context.session[:something_useful]
     }
  end
end

This would put a prop of somethingUseful in the railsContext passed to all react_component and redux_store calls.

Since you can't access the rails session from JavaScript (or other values available in the view rendering context), this might useful.

So why not put this in props?

The advantage to the railsContext is that this is consistent for all calls.

Feedback?

[justin808]

@alexfedoseev @martyphee @jbhatab This is ready to merge once I add tests for the new functionality. The work on the view helper and the JS libraries should be complete, as well as to the /spec/dummy sample app.

Any comments on the API?

I hope to merge this to master on Saturday!

[coveralls]

Coverage increased (+0.3%) to 87.784% when pulling 3d1c460 on add-location-to-redux-store-generator into 1081266 on master.

[jbhatab]

@justin808 I think the api for the RenderingExtension thing is a bit odd. The concept is fine for flexibility. Can we just have extra params that are passed in on the helper calls get passed in automatically? Like a wildcard args situation.

[justin808]

Review status: 0 of 21 files reviewed at latest revision, 25 unresolved discussions.

---

app/helpers/react_on_rails_helper.rb, line 149 [r3] (raw file):
this will make sure that we've rendered a hidden div with the contextual information exactly ONCE.

---

app/helpers/react_on_rails_helper.rb, line 172 [r3] (raw file):
this will make sure that we've rendered a hidden div with the contextual information exactly ONCE.

---

app/helpers/react_on_rails_helper.rb, line 242 [r3] (raw file):
Only once! Else just send back the dom string passed.

---

app/helpers/react_on_rails_helper.rb, line 255 [r3] (raw file):
We put the context div first.

---

app/helpers/react_on_rails_helper.rb, line 291 [r3] (raw file):
put the railsContext in a variable, used in the JS call to server render

---

app/helpers/react_on_rails_helper.rb, line 348 [r3] (raw file):
The definitive list of what goes in the rails context

---

node_package/src/clientStartup.js, line 35 [r3] (raw file):
We consistently call generator functions with the railsContext parameter

---

node_package/src/clientStartup.js, line 51 [r3] (raw file):
We consistently call generator functions with the railsContext parameter

---

node_package/src/clientStartup.js, line 73 [r3] (raw file):
We consistently call generator functions with the railsContext parameter

---

node_package/src/clientStartup.js, line 106 [r3] (raw file):
only had one initializeStore function, so no need to make it a parameter

---

node_package/src/createReactElement.js, line 25 [r3] (raw file):
This change makes tracing now log the props and context.

I'll have to experiment to see if this is overkill.

---

node_package/src/createReactElement.js, line 33 [r3] (raw file):
yep, call with the railsContext, and not location.

BREAKING CHANGE RIGHT HERE

Yes, going from a string to an object.

CC: @jbhatab, @thewoolleyman, @martyphee, @alexfedoseev

---

spec/dummy/app/controllers/pages_controller.rb, line 5 [r3] (raw file):
example of having value in session to put into rails context

---

spec/dummy/client/app/components/HelloWorld.jsx, line 14 [r3] (raw file):
optional, bc we do not get this for standard invocation, only generator functions

---

spec/dummy/client/app/components/HelloWorld.jsx, line 57 [r3] (raw file):
We only get the RailsContext for generator functions.

---

spec/dummy/client/app/components/HelloWorldContainer.jsx, line 9 [r3] (raw file):
@alexfedoseev @robwise Is this a good pattern to pass in the railsContext as a 3rd param to the container?

---

spec/dummy/client/app/components/HelloWorldContainer.jsx, line 23 [r3] (raw file):
Here's where we map the state to props for railsContext

---

spec/dummy/client/app/components/HelloWorldRedux.jsx, line 10 [r3] (raw file):
In the redux case, I'm always using railsContext

---

spec/dummy/client/app/components/RailsContext.jsx, line 1 [r3] (raw file):
Make a simple table of the context values for test verification and demonstration

---

spec/dummy/client/app/reducers/RailsContextReducer.jsx, line 1 [r3] (raw file):
@alexfedoseev does it make sense to have a reducer if there's no actions? I guess this is how the state gets put together.

---

spec/dummy/client/app/reducers/reducersIndex.jsx, line 8 [r3] (raw file):
This is how we get the railsContext into the redux store.

---

spec/dummy/client/app/startup/ClientReduxApp.jsx, line 20 [r3] (raw file):
@alexfedoseev Please confirm this is the best way to get the railsContext put into the props for redux.

Sort of interesting that we slam the param in, and then make it its own "reducer domain".

Maybe there's a better pattern since the the rails_context really is immutable?

---

spec/dummy/client/app/startup/ServerReduxApp.jsx, line 21 [r3] (raw file):
The server side example code is duplicated. Wondering if there's a better way.

---

spec/dummy/client/app/startup/ServerRouterApp.jsx, line 11 [r3] (raw file):
This is how we get the router location needed.

---

spec/dummy/config/initializers/react_on_rails.rb, line 9 [r3] (raw file):
@martyphee This is a custom extension to the rails_context.

---

Comments from the review on Reviewable.io

[justin808]

@jbhatab You already have props for that. So I don't understand your use case.

---

Review status: 0 of 21 files reviewed at latest revision, 25 unresolved discussions.

---

Comments from the review on Reviewable.io

[justin808]

AFAIK, my only remaining task on this PR is to update the /spec/dummy tests to confirm the values in the displayed table.

---

Review status: 0 of 21 files reviewed at latest revision, 25 unresolved discussions.

---

Comments from the review on Reviewable.io

[coveralls]

Coverage increased (+0.3%) to 87.784% when pulling d7ce593 on add-location-to-redux-store-generator into 1081266 on master.

[alex35mil]

@justin808 @jbhatab

---

Reviewed 13 of 17 files at r2, 4 of 5 files at r3, 4 of 5 files at r4.
Review status: all files reviewed at latest revision, 25 unresolved discussions.

---

app/helpers/react_on_rails_helper.rb, line 348 [r3] (raw file):
This looks good! 👍

---

spec/dummy/client/app/components/HelloWorldContainer.jsx, line 9 [r3] (raw file):
I don't see any downsides here.

---

spec/dummy/client/app/reducers/RailsContextReducer.jsx, line 1 [r3] (raw file):
It makes sense, we must have reducer to put some initial data (even if there's no actions).

---

spec/dummy/client/app/startup/ClientReduxApp.jsx, line 20 [r3] (raw file):
I'm not a big fan of mutating args (I usually use Object.assign to merge objects), but it seems fine.

---

spec/dummy/client/app/startup/ServerReduxApp.jsx, line 21 [r3] (raw file):
Abstract it into module composeInitialState, which will be used by client and server startup components.

---

Comments from the review on Reviewable.io

[justin808]

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

---

spec/dummy/client/app/startup/ClientReduxApp.jsx, line 20 [r3] (raw file):
Done.

export default (props, railsContext) => {
  const combinedReducer = combineReducers(reducers);
  const combinedProps = Object.assign({}, props, railsContext);
  const store = applyMiddleware(middleware)(createStore)(combinedReducer, combinedProps);

---

Comments from the review on Reviewable.io

[justin808]

Thanks Alex!

---

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

---

spec/dummy/client/app/startup/ServerReduxApp.jsx, line 21 [r3] (raw file):
Whole file or just the creation of the store, to abstract.

---

Comments from the review on Reviewable.io

[justin808]

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

---

spec/dummy/spec/features/rails_context_spec.rb, line 1 [r5] (raw file):
This file checks client side values

---

spec/dummy/spec/requests/server_render_check_spec.rb, line 61 [r5] (raw file):
we're checking that we have the correct values on the server side

---

Comments from the review on Reviewable.io

[coveralls]

Coverage increased (+0.3%) to 87.784% when pulling 3834945 on add-location-to-redux-store-generator into 8482885 on master.

[coveralls]

Coverage increased (+0.3%) to 87.784% when pulling 0a09b81 on add-location-to-redux-store-generator into 8482885 on master.

[martyphee]

Sorry for the late response. This won’t solve our current needs. We still need a way load the proper i18n file for server side rendering.

[jbhatab]

@martyphee you can't pass the locale file into with this?

[martyphee]

Maybe. Right now we're depending on a global i18n object which is a wrapper around Jed. I would have to pass the json string into the component and initialize a Jed instance in each component or maybe pass as property to the children. I haven't had a chance to play around with it. May original PR just included the below file inline so that it was initialized before trying to render the component on the server.

Jed is initialized like so in the i18n file being downloaded by the client.

//= require jed/jed
//= require locale/i18n

//= require_self

jed = new Jed({
   "domain": "orderwebjsx",
   "locale_data": {
      "orderwebjsx": {
         "": {
            "domain": "orderwebjsx",
            "plural_forms": "nplurals=2; plural=(n != 1);",
            "lang": "de"
         },
         "%(name)s": [
            "%(name)s"
         ],
         "A Simple, Fixed Fee Per Delivery": [
            "Eine niedrige, fixe Gebühr pro Lieferung"
         ],
         "Accepted": [
            "Akzeptiert"
         ],
         "Almost ready": [
            "Fast fertig"
         ],

[justin808]

@martyphee, @jbhatab You should be able to put these localization setups in separate files using the locale name in the file name (or as the whole file name). Then just do a require (not an import) based on the file appropriate for the locale.

Since we cache the "context" of the whole server bundle file, this might be slightly more performant.

Any reason that won't work?

[martyphee]

The translations are needed for server rendering of the React component. Somehow I have to have i18n initialized when the component is rendered on the server.

I might be looking at this again very soon. Looks like it's falling on me to rewrite a large portion of the site.