Skip to content

Ensure render of I18nProvider in async scenarios#839

Merged
tricoder42 merged 3 commits into
lingui:mainfrom
Bertg:ensure_i18n_provider_make_context_in_async_scenarios
Nov 9, 2020
Merged

Ensure render of I18nProvider in async scenarios#839
tricoder42 merged 3 commits into
lingui:mainfrom
Bertg:ensure_i18n_provider_make_context_in_async_scenarios

Conversation

@Bertg
Copy link
Copy Markdown
Contributor

@Bertg Bertg commented Nov 9, 2020

This is related to #834

This PR solves 2 issues:

  • Making sure the component gets re-rendered when needed
  • Provide a warning to the developer

Some more thoughts about this PR and problem:

  • It is perfectly possible for the warning to appear even if the component will render.
  • It would be better to always render. We can add a warning to the developer that only defaults will be rendered. This requires the developer to make sure a language is activated before they render the component. We could even choose to completely throw if i18n is not activated yet, unless the developer set a prop indicating they are OK with defaults.
  • forceRenderOnLocaleChange should be separated from dontRenderIfNoLocaleIsActivated
  • ideally there should be a i18n.on("activated" which also triggers when first observed (if already triggered before). That way the extra call to setRenderKey in the effect is never needed.

@vercel
Copy link
Copy Markdown

vercel Bot commented Nov 9, 2020

@Bertg is attempting to deploy a commit to the LinguiJS Team on Vercel.

A member of the Team first needs to authorize it.

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 9, 2020

Codecov Report

Merging #839 (934639d) into main (a61c57a) will increase coverage by 0.02%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #839      +/-   ##
==========================================
+ Coverage   84.25%   84.27%   +0.02%     
==========================================
  Files          38       38              
  Lines        1251     1259       +8     
  Branches      332      334       +2     
==========================================
+ Hits         1054     1061       +7     
  Misses        117      117              
- Partials       80       81       +1     
Impacted Files Coverage Δ
packages/react/src/I18nProvider.tsx 83.87% <91.66%> (+1.26%) ⬆️

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 a61c57a...934639d. Read the comment docs.

@tricoder42
Copy link
Copy Markdown
Contributor

Perfect, thanks!

ideally there should be a i18n.on("activated"

Let's try it as it is and figure out how to improve it later. I guess we could add a signal activated or loaded which would be triggered after message catalog for active locale is loaded and available.

@tricoder42 tricoder42 merged commit cd2816a into lingui:main Nov 9, 2020
@tricoder42 tricoder42 linked an issue Nov 10, 2020 that may be closed by this pull request
@Bertg
Copy link
Copy Markdown
Contributor Author

Bertg commented Nov 10, 2020

Quick question... may I be added to the contributor list? :)

@tricoder42
Copy link
Copy Markdown
Contributor

@Bertg I don't think we have an explicit list of contributors 🤔 The contributors section in README is generated automatically from repository contributors. I guess it's just cached and you should appear there in few days.

And if I haven't said it explicitly, thank you for your help! I really appreaciate everyone who's willing to spend their free time contributing to this project 🙏

@Bertg
Copy link
Copy Markdown
Contributor Author

Bertg commented Nov 10, 2020

You're welcome :)

Actually felt kind of embarrassed asking, but this is a project I would be proud of having my name attached to.

@tricoder42
Copy link
Copy Markdown
Contributor

Yeah, this is a valid point 👍 We could use GitHub Action to automatically generate list of contributors after each pull request. I'm gonna check it later. GitHub Actions are really powerfull 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Edge case may leave I18nProvider un-rendered

2 participants