Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add initialize prop to LocaizeProvider to support SSR #127

Merged
merged 2 commits into from
Sep 12, 2018

Conversation

ryandrewjohnson
Copy link
Owner

@ryandrewjohnson ryandrewjohnson commented Sep 11, 2018

Proposed fix for #125 and #103 both related to server side rendering.

Issue:

The library currently doesn't work with server side rendering, as the initial context for LocalizeProvider has no values for languages, or translations until the initialize action is called.

Since no the state changes triggered by the initialize action will be tracked on the server this results in the empty context. This causes issues when the server attempts to render components with translations, and results in errors being thrown because of missing languages, and translation data.

Solution:

Allow for passing initialize payload as a prop to LocalizeProvider. If using server side rendering this would be the required way to initialize, and would need to be done on the server and the client.

Example:

// from server
const result = ReactDOMServer.renderToString(
      <LocalizeProvider initialize={{
        languages: [
          { name: "English", code: "en" },
          { name: "French", code: "fr" }
        ],
        translation: {
          hello: ['hello', 'alo']
        },
        options: {
          defaultLanguage: "en",
          renderToStaticMarkup: ReactDOMServer.renderToStaticMarkup
        }
      }}>
        <LocalizedApp />
      </LocalizeProvider>
    );

// from client
ReactDOM.hydrate(
    <LocalizeProvider initialize={{
        languages: [
          { name: "English", code: "en" },
          { name: "French", code: "fr" }
        ],
        translation: {
          hello: ['hello', 'alo']
        },
        options: {
          defaultLanguage: "en",
          renderToStaticMarkup: ReactDOMServer.renderToStaticMarkup
        }
      }}>
        <LocalizedApp />
   </LocalizeProvider>
)

Instead of repeating the initialize payload on both the server, and client you could use a workflow that is well known for Redux and SSR where the initialState is stored in a window object. That is really up to the user to decide.

If using server side rendering this will be used in place of the initialize action that would usually be called from a component lifecycle method. If not using server side rendering this prop could also be used in place of the initialize action call, that is up to the user, and what use case works best for them.

@codecov
Copy link

codecov bot commented Sep 11, 2018

Codecov Report

Merging #127 into master will increase coverage by 0.81%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #127      +/-   ##
=========================================
+ Coverage   95.58%   96.4%   +0.81%     
=========================================
  Files           6       6              
  Lines         249     250       +1     
  Branches       75      76       +1     
=========================================
+ Hits          238     241       +3     
+ Misses         10       8       -2     
  Partials        1       1
Impacted Files Coverage Δ
src/localize.js 96.7% <ø> (ø) ⬆️
src/utils.js 100% <ø> (ø) ⬆️
src/LocalizeProvider.js 89.65% <100%> (+3.94%) ⬆️
src/LocalizeContext.js 82.35% <0%> (+5.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2da3fb...30ff6d0. Read the comment docs.

@ramyma
Copy link
Contributor

ramyma commented Sep 11, 2018

I tried it on my project and it works like a charm, both SSR and client side rendering.

Thanks!

@ramyma
Copy link
Contributor

ramyma commented Sep 11, 2018

I think it's ready for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants