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

docs: various docs improvements #1781

Merged
merged 9 commits into from
Dec 20, 2023
Merged

Conversation

vonovak
Copy link
Collaborator

@vonovak vonovak commented Oct 12, 2023

Description

I started reading the docs top to bottom, this is the first batch of updates.

Mostly changes to wording, a / the changes, sometimes rewriting stuff for clarity.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Fixes # (issue)

Checklist

  • I have read the CONTRIBUTING and CODE_OF_CONDUCT docs
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@vercel
Copy link

vercel bot commented Oct 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
js-lingui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 20, 2023 1:20pm

@github-actions
Copy link

github-actions bot commented Oct 12, 2023

size-limit report 📦

Path Size
./packages/core/dist/index.mjs 2.81 KB (0%)
./packages/detect-locale/dist/index.mjs 723 B (0%)
./packages/react/dist/index.mjs 1.67 KB (0%)
./packages/remote-loader/dist/index.mjs 7.26 KB (0%)

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (04b7cef) 76.27% compared to head (69195f6) 76.38%.
Report is 10 commits behind head on main.

❗ Current head 69195f6 differs from pull request most recent head 90c6eda. Consider uploading reports for the commit 90c6eda to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1781      +/-   ##
==========================================
+ Coverage   76.27%   76.38%   +0.11%     
==========================================
  Files          81       81              
  Lines        2074     2071       -3     
  Branches      529      529              
==========================================
  Hits         1582     1582              
+ Misses        380      377       -3     
  Partials      112      112              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@timofei-iatsenko timofei-iatsenko self-requested a review October 12, 2023 08:19
website/docs/guides/excluding-build-files.md Outdated Show resolved Hide resolved
website/docs/guides/message-extraction.md Outdated Show resolved Hide resolved
website/docs/ref/cli.md Outdated Show resolved Hide resolved
website/docs/tutorials/cli.md Outdated Show resolved Hide resolved
website/docs/tutorials/explicit-vs-generated-ids.md Outdated Show resolved Hide resolved
@andrii-bodnar
Copy link
Contributor

@vonovak could you please address the discussions and rebase the branch? Thank you!

@andrii-bodnar andrii-bodnar marked this pull request as draft November 17, 2023 10:04
@andrii-bodnar andrii-bodnar marked this pull request as ready for review November 28, 2023 20:05
Copy link
Contributor

@andrii-bodnar andrii-bodnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vonovak thank you!

@vonovak
Copy link
Collaborator Author

vonovak commented Nov 29, 2023

@andrii-bodnar @thekip I'd like to hear your opinion on this:

(1) version control: this is currently documented in two places: cli and Excluding message catalog build files

I think we should have this topic documented in one place (the latter) but more importantly, the guides contradict each other:

the first says: "What you do need to keep in VCS are the JSON files (locale//.json) that contain the messages for translators."

the second says: "lingui extract command creates temporary message catalogs per each source file. Also, lingui compile command generates compiled message catalogs from source ones. All these files can be safely ignored from VCS and linters.

Can be safely ignored because these files must be created every time you deploy to production"

We should unify the recommended approach. What should it look like?

(2) a different topic but potentially related: in the home page we say "If your team needs to edit source texts without developer involvement... – we've got you covered."

I think it's worth documenting how this should be done. If I understand it correctly (and I'm not sure), the way this should be done is that once you define a message (e.g. t`No user returned from database`) then the developer should not touch that string. When a request comes in to change this to "No user returned from API" then some project manager can change the string (for example) in Crowdin, but the app itself should keep the old id?
If a developer changes that string to the new one (t`No user returned from API`), the generated id of the string changes. If the dev then deploys to prod, there will be missing translation for other languages (potentially even for the source lang), do I see it correctly?

Thank you!

@andrii-bodnar
Copy link
Contributor

Hi @vonovak, thanks for bringing up these issues!

(1) - I think it would be good to keep the version control information in one place (the "Excluding message catalog build files" article). For the CLI article, we can keep just a cross-link to the first article.

(2) - This approach only works well with the explicit IDs if the developer only keeps keys in the source code. Then they can pull the externally edited sources into their message catalogs and compile them. If the keys change, the translations will probably be lost.

@timofei-iatsenko
Copy link
Collaborator

the second says: "lingui extract command creates temporary message catalogs per each source file. Also, lingui compile command generates compiled message catalogs from source ones. All these files can be safely ignored from VCS and linters.

This is extremely outdated sentence In some first version of extractor implementation it indeed created a json file with extracted messages next to each source file. So this is about it, and this is outdated. Extract command no longer create a temporary files and keep them only in memory. So this could be dropped.

The locale/_build folder and locale//.js (compiled catalogs) are safe to be ignored by your VCS. What you do need to keep in VCS are the JSON files (locale//.json) that contain the messages for translators. The JavaScript functions that return the actual translations when your app runs in production are created from those JSON files. See Excluding build files guide for more info.

This paragraph about 2 different type of files. Catalog sources such as *.po files (for po-formatter) or *.json files (for mimimal and lingui formatters), and theirs compiled artifacts produced by "lingui compile" if you use it instead of webpack's or vite's loaders. So the compiled artifacts might be safely ignored if you have a compile step in the build pipeline.

So this part is also outdated because we advocate to use po format time over json, but this is not reflected in this paragrpaph.

I think it's worth documenting how this should be done. If I understand it correctly (and I'm not sure), the way this should be done is that once you define a message (e.g. tNo user returned from database) then the developer should not touch that string. When a request comes in to change this to "No user returned from API" then some project manager can change the string (for example) in Crowdin, but the app itself should keep the old id?
If a developer changes that string to the new one (tNo user returned from API) and then deploys to prod, there will be missing translation for other languages, do I see it correctly?

Yes, you are right. As Andrii mentioned this works better with explicit id's, but still possible with natural language as id as well. It's just bring a bit of mess, because you will have one string in source code and will see different string in runtime. But since we use for default language the same catalogs as for any other language you freely can change it without touching the source code.

@andrii-bodnar andrii-bodnar merged commit 87e0ef4 into lingui:main Dec 20, 2023
14 checks passed
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.

3 participants