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 render of I18nProvider in async scenarios #839

Conversation

Bertg
Copy link
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

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

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
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
Contributor Author

Bertg commented Nov 10, 2020

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

@tricoder42
Copy link
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
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
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