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

feat: add meta with codes on warnings to allow conditional logging #1826

Merged

Conversation

delta-9
Copy link
Contributor

@delta-9 delta-9 commented Dec 29, 2024

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included
  • commit message and code follows the Developer's Certification of Origin

Checklist (for documentation change)

  • only relevant documentation part is changed (make a diff before you submit the PR)
  • motivation/reason is provided
  • commit message and code follows the Developer's Certification of Origin

@delta-9 delta-9 force-pushed the feat-add-option-to-override-warn-logger branch 3 times, most recently from 9f9deb6 to 914ebad Compare December 29, 2024 12:35
@delta-9 delta-9 closed this Dec 29, 2024
@delta-9 delta-9 force-pushed the feat-add-option-to-override-warn-logger branch from 914ebad to ff509ba Compare December 29, 2024 12:49
@delta-9 delta-9 reopened this Dec 29, 2024
@delta-9 delta-9 force-pushed the feat-add-option-to-override-warn-logger branch from e478b8d to 57f58fe Compare December 29, 2024 12:53
@delta-9 delta-9 changed the title feat: add option to override warn logger feat: add meta with codes on warnings to allow conditional logging Dec 29, 2024
@adrai
Copy link
Member

adrai commented Dec 29, 2024

not necessary, since you can now inject your own logger: i18next/react-i18next-gitbook#150 (comment)

@delta-9 delta-9 force-pushed the feat-add-option-to-override-warn-logger branch from 57f58fe to 2b3bd6e Compare December 29, 2024 12:54
@delta-9
Copy link
Contributor Author

delta-9 commented Dec 29, 2024

@adrai I saw your changes just now, cool!, this PR now just adds meta so I can filter what to log or not

@adrai
Copy link
Member

adrai commented Dec 29, 2024

I suggest, you move this logic out to your custom logger... else this will bloat the code size even more... //cc @VIKTORVAV99

@delta-9 delta-9 force-pushed the feat-add-option-to-override-warn-logger branch from 2b3bd6e to 8848b1e Compare December 29, 2024 13:43
@delta-9
Copy link
Contributor Author

delta-9 commented Dec 29, 2024

I suggest, you move this logic out to your custom logger... else this will bloat the code size even more... //cc @VIKTORVAV99

fair enough, I reworked the changes to includes only what is needed to do that from the logger

@adrai
Copy link
Member

adrai commented Dec 29, 2024

Do you really need extra error codes? The whole i18next core code base has no error codes...

@coveralls
Copy link

coveralls commented Dec 29, 2024

Coverage Status

coverage: 95.797% (-0.6%) from 96.386%
when pulling 4be7a37 on delta-9:feat-add-option-to-override-warn-logger
into ff509ba on i18next:master.

@delta-9 delta-9 marked this pull request as ready for review December 29, 2024 14:05
@delta-9
Copy link
Contributor Author

delta-9 commented Dec 29, 2024

Do you really need extra error codes? The whole i18next core code base has no error codes...

the error codes is to be able to identify the warnings in the code.
it also adds the i18nkey to the warning logged so its easier to find the culprit trans.

the i18nkey also allow to add a logic to warnOnce per key in the custom logger, now it warn on each renders.

for the code I could do some string regexp on the messages to figure out the cause of the error,
and for the i18nkey, Its also possible to debug following the stack trace and put some trace until I find the right call.

@VIKTORVAV99
Copy link
Contributor

VIKTORVAV99 commented Dec 29, 2024

Maybe there are benefits from this but I would like you to consider two things:

First this will increase the overhead and bundle size for all users of i18next, I've been trying to push this bundle down as it's one of the bigger parts of our app we can't change at the moment.

Secondly it feels like this is just duplicating the error message. If we do this I would switch to something what react does and only log the error codes with a link to look up what they mean. But that requires other changes as well.

That is just my thought on the matter though but I would really like to avoid duplicating any information or functionality for the sake of making it a little more convenient for one project or developer.

@delta-9 delta-9 force-pushed the feat-add-option-to-override-warn-logger branch 2 times, most recently from 33243a3 to 6b11535 Compare December 29, 2024 18:56
@rawpixel-vincent
Copy link
Contributor

rawpixel-vincent commented Dec 29, 2024

I replaced the error map with a tuple, and reduced duplicates, it's adding 201 bytes to react-i18next.min.js build compared to current master

moving the error messages to the documentation might be a good idea. They can be better explained in the docs, and changed without worrying of breaking someone code.

src/utils.js Outdated Show resolved Hide resolved
@delta-9
Copy link
Contributor Author

delta-9 commented Dec 29, 2024 via email

@delta-9 delta-9 force-pushed the feat-add-option-to-override-warn-logger branch from 6b11535 to 8fe0b03 Compare December 30, 2024 05:32
@delta-9
Copy link
Contributor Author

delta-9 commented Dec 30, 2024

good news @adrai I moved the error code to a type declaration file so its not in the code.

the minify js is now 20 bytes less than master 🎉

@delta-9 delta-9 force-pushed the feat-add-option-to-override-warn-logger branch 2 times, most recently from 8375d3d to 3faabc8 Compare December 30, 2024 06:07
@delta-9
Copy link
Contributor Author

delta-9 commented Dec 30, 2024

added types for the warning args along an example of usage in the logger plugin

edit: typo in example

@delta-9 delta-9 force-pushed the feat-add-option-to-override-warn-logger branch from 3faabc8 to 8ee8463 Compare December 30, 2024 06:14
@adrai
Copy link
Member

adrai commented Dec 30, 2024

@delta-9 delta-9 force-pushed the feat-add-option-to-override-warn-logger branch from 8ee8463 to 4be7a37 Compare December 30, 2024 10:09
@delta-9
Copy link
Contributor Author

delta-9 commented Dec 30, 2024

it should be fixed, but I can't make the test fail locally, npm run test pass

@adrai adrai merged commit 3cd025f into i18next:master Dec 30, 2024
7 of 9 checks passed
@adrai
Copy link
Member

adrai commented Dec 30, 2024

it's included in v15.4.0

alexandresoro pushed a commit to alexandresoro/ouca-backend that referenced this pull request Dec 30, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [react-i18next](https://github.com/i18next/react-i18next) | dependencies | minor | [`15.3.0` -> `15.4.0`](https://renovatebot.com/diffs/npm/react-i18next/15.3.0/15.4.0) |

---

### Release Notes

<details>
<summary>i18next/react-i18next (react-i18next)</summary>

### [`v15.4.0`](https://github.com/i18next/react-i18next/blob/HEAD/CHANGELOG.md#1540)

[Compare Source](i18next/react-i18next@v15.3.0...v15.4.0)

feat: add meta with codes on warnings to allow conditional logging [1826](i18next/react-i18next#1826)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS44Ni4wIiwidXBkYXRlZEluVmVyIjoiMzkuODYuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiZGVwZW5kZW5jaWVzIl19-->

Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/434
Reviewed-by: Alexandre Soro <[email protected]>
Co-authored-by: renovate <[email protected]>
Co-committed-by: renovate <[email protected]>
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.

5 participants