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

change to return false when a nullable argument is passed #1635

Merged

Conversation

raccoonback
Copy link
Contributor

@raccoonback raccoonback commented Aug 4, 2021

I have used i18next, react-i18next.
However, when using the i18n object to determine the existence of the key, I found that passing one nullish factor(undefined or null, or '') results in the error below:

TypeError
Cannot read property 'forEach' of undefined

I think i18n.exist(undefined or null, or '') should return false.

Similar to mine, there will be many cases that deliver nullish factors, and I think it is obvious that nullish factors do not exist as keys in the resource.

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included

Checklist (for documentation change)

  • only relevant documentation part is changed (make a diff before you submit the PR)
  • motivation/reason is provided

@coveralls
Copy link

coveralls commented Aug 4, 2021

Coverage Status

Coverage increased (+0.01%) to 93.216% when pulling f9f3817 on raccoonback:check-nullable-args-in-translator-exist into b371a5c on i18next:master.

@jamuhl
Copy link
Member

jamuhl commented Aug 4, 2021

wouldn't it be cleaner to add a check in the exists function itself than in the reused resolve? Like it's already done for the translate here: https://github.com/i18next/i18next/blob/master/src/Translator.js#L87

@raccoonback
Copy link
Contributor Author

@jamuhl

I like your opinion,
but wouldn't it be better to deal with common within the resolve() function because this error is from the resolve() function?

because nullable values(undefined or null, or '') can be passed to keys parameter elsewhere using the resolve() function`.

@jamuhl
Copy link
Member

jamuhl commented Aug 4, 2021

resolve is internal...currently translate, exist are "public" functions - as translate handles the return value for nullish already it seems logical for me to let exists handle it itself too

@raccoonback
Copy link
Contributor Author

@jamuhl
Yes, I understand. I'll change it.

@raccoonback raccoonback force-pushed the check-nullable-args-in-translator-exist branch from 9218662 to f57c097 Compare August 4, 2021 09:00
@raccoonback
Copy link
Contributor Author

@jamuhl
I've reflected all the reviews. Please check it again.

src/Translator.js Outdated Show resolved Hide resolved
@raccoonback raccoonback requested a review from jamuhl August 4, 2021 09:24
@raccoonback
Copy link
Contributor Author

raccoonback commented Aug 4, 2021

@jamuhl
I've reflected the your review. Please check it one more. 😃

@adrai
Copy link
Member

adrai commented Aug 4, 2021

lgtm

Copy link
Member

@jamuhl jamuhl left a comment

Choose a reason for hiding this comment

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

lgtm

@jamuhl jamuhl merged commit d0dc508 into i18next:master Aug 4, 2021
@jamuhl
Copy link
Member

jamuhl commented Aug 4, 2021

Guess this could be released with next version...?

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.

None yet

4 participants