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

Ensure that addDefaultTranslation gets called #99

Merged
merged 3 commits into from
Jun 25, 2018

Conversation

thchia
Copy link
Collaborator

@thchia thchia commented Jun 23, 2018

For #97

This change implements a fix based on the solution described in the linked issue.

Changes

utils.get

I added a helper to access values nested in objects. This is just to avoid verbose checks like:

if (obj && obj.a && obj.a.b) return obj.a.b.c

// instead

const val = get(obj, 'a.b.c', 'fallback Value')

Inspired by lodash.get, but probably much simpler.

Translate.js

I added some logic to check if the defaultLanguage has gone from undefined to something. It looks a bit verbose but there is a need to check both props.context and props.options I think. @ryandrewjohnson I don't know if there is a need to address the case where the language changes from one language to another?

Translate.test.js

In getTranslateWithContext(), I amended the default language to better reflect how this value is arrived in real use.

As far as I can tell, in real use, defaultLanguage may start off as undefined, before initialize() has completed. It is impossible to recreate this in getTranslateWithContext() because it has a default to ensure that defaultLanguage is always defined.

To simulate real use, you can explicitly pass an empty language option array to the initialState in getTranslateWithContext(initialState). This then throws because the default I mentioned above is accessing languages[0].code. So i just added a check to see if there is an element at languages[0], otherwise return undefined.

// before
state.options.defaultLanguage || getLanguages(localizeState)[0].code

// after
state.options.defaultLanguage || (getLanguages(localizeState)[0] && getLanguages(localizeState)[0].code)

This matches how the default language is determined in LocalizeContext:

// in getContextPropsFromState()

const defaultLanguage = options.defaultLanguage || (languages[0] && languages[0].code)

// Note the check for languages[0]

To simulate the defaultLanguage changing from undefined to something, in the test case I created Translate with an empty languages array, then explicitly set the options prop on Translate to contain a language. Then we can assert that addTranslationForLanguage() was called.

@codecov
Copy link

codecov bot commented Jun 23, 2018

Codecov Report

Merging #99 into master will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
+ Coverage   95.33%   95.49%   +0.15%     
==========================================
  Files           6        6              
  Lines         236      244       +8     
  Branches       68       74       +6     
==========================================
+ Hits          225      233       +8     
  Misses         10       10              
  Partials        1        1
Impacted Files Coverage Δ
src/Translate.js 100% <100%> (ø) ⬆️
src/utils.js 100% <100%> (ø) ⬆️

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 7b18367...ca4d816. Read the comment docs.

@ryandrewjohnson
Copy link
Owner

@thchia I don't think we need to track language change, as it should have not impact on default language. Going to merge this one in.

@ryandrewjohnson ryandrewjohnson merged commit 1a726d4 into ryandrewjohnson:master Jun 25, 2018
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.

2 participants