Initialize ignore translate

[thchia]

For #89.

Summary

This change allows ignoreTranslateChildren to be passed in the initialize() call. It allows global setting of this flag, which determines whether or not Translate children should be added into the default language.

Review

• In LocalizeContext I ran into issues with flow. Because ignoreTranslateChildren is optional in the initialize() options, but required in the context object, flow was complaining that a boolean or undefined cannot be assigned to a boolean. It did this even after my attempts to narrow the type, so the resulting logic is somewhat verbose. Any suggestions welcome (I'm not familiar with flow).

• I left a note in Translate regarding the logic that determines whether to proceed with addDefaultTranslation because the code might look unnecessarily complex. What I naively did at first (in pseudocode):

// Translate.js

addDefaultTranslation(context) {

  ...

  if (props.options.ignoreTranslateChildren || context.ignoreTranslateChildren) {
    return; // don't translate
  }

  translate(...);
}

This will not work because if <Translate options={{ ignoreTranslateChildren: false }} />, it will be overriden by a potential context.ignoreTranslateChildren === true that was set from initialize(). In fact, whatever is set on Translate should have the final say. Since undefined and false are falsy, there is a need to do an explicit check:

// Translate.js

addDefaultTranslation(context) {

  ...

  const translatePropSaysIgnore = props.options.ignoreTranslateChildren
  const translatePropUndefinedAndContextSaysIgnore = (options.ignoreTranslateChildren == null) && context.ignoreTranslateChildren;

  if (translatePropSaysIgnore || translatePropUndefinedAndContextSaysIgnore) {
    return; // don't translate
  }

  translate(...);
}

Again, any suggestions of how to make this more concise are welcome. I have put a test in anyway for good measure so it should be pretty safe.

[codecov]

Codecov Report

Merging #91 into master will increase coverage by 0.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
+ Coverage   92.75%   92.89%   +0.13%     
==========================================
  Files           6        6              
  Lines         207      211       +4     
  Branches       59       61       +2     
==========================================
+ Hits          192      196       +4     
  Misses         13       13              
  Partials        2        2

Impacted Files	Coverage Δ
src/localize.js	96.51% <ø> (ø)	⬆️
src/LocalizeContext.js	76.47% <100%> (+1.47%)	⬆️
src/Translate.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 126ed12...db2127b. Read the comment docs.

[ryandrewjohnson]

@thchia I have pushed some changes the simplifies some of that logic that you mentioned in both LocalizeContext and Translate. All the tests appear to be passing, but if you can have a look on your end and make sure it looks good.

If all is well we can merge.

[thchia]

This seems confusingly named, since it is checking that children is undefined. It also means the if condition below reads as if (hasChildren...) return which isn't the case?

[ryandrewjohnson]

Yep that’s my bad 🤗. Will look at renaming some of these.

[thchia]

I may be overthinking this one, but if one day it is decided that the default ignoreTranslateChildren is true, it will override a potential false being sent in by the user. Since the default is determined by the library itself probably the best thing is to add a test to help current/future maintainers.

[thchia]

Actually it looks like this will be tricky to test due to the Enzyme problem with Context... It seems like it's not possible to trigger the whole flow of:

1. Instantiating the Provider (with default ignoreTranslateChildren: true)
2. Calling initialize() with ignoreTranslateChildren: false
3. Instantiating Translate and checking that the child was rendered.

[ryandrewjohnson]

Yea I think this can be updated to evaluate similar to how I handled ignoreTranslateChildren in the Translate component, which should handle this scenario.

[ryandrewjohnson]

@thchia I've pushed up changes that address the two comments you made. I'm going to go ahead and merge this one so we can get it published.