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 a new option fallbackRootWithEmptyString #1441

Merged
merged 2 commits into from
Jan 21, 2022

Conversation

PeterAlfredLee
Copy link
Contributor

This PR introduces a new option fallbackRootWithEmptyString. This option decides a i18n message should fallback to root or not if it's a empty string.

Currently, the fallback root check logic is like this:

 !val && !isNull(this._root) && this._fallbackRoot

This means that: in the case that the val is an empty string(''), it will also fallback to root. This implementation is different from the one in version 9.x(in i18n 9.x, the empty string will not trigger fallback root). Besides that, this implementation may make it difficult if the user want to bypass the fallback operations of some value.

This PR introduces a new option fallbackRootWithEmptyString whose default value is true. It means the default logic of fallback root is still like this :

 !val && !isNull(this._root) && this._fallbackRoot

This means the PR is not a breaking change: by default, the fallback root logic will work in the existing logic.

When setting the fallbackRootWithEmptyString to be falsem the fallback root logic will become like this:

isNull(val) && !isNull(this._root) && this._fallbackRoot

This means the emptr string('') will not trigger fallback root.

Besides that, I didn't add this option in the api.md, cause I want to hear your ideas about this new option first. :) @kazupon

Copy link
Owner

@kazupon kazupon left a comment

Choose a reason for hiding this comment

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

Oh, I have not realized I was having a BREAKING CHANGE in v9 😣
This option should be useful when migrating from v8 to v9 for some users.
This should be documented in the API docs (api.md).

And we need to add the typescript type definition as well.
https://github.com/kazupon/vue-i18n/blob/v8.x/types/index.d.ts

@PeterAlfredLee
Copy link
Contributor Author

Oh, I have not realized I was having a BREAKING CHANGE in v9 😣 This option should be useful when migrating from v8 to v9 for some users. This should be documented in the API docs (api.md).

And we need to add the typescript type definition as well. https://github.com/kazupon/vue-i18n/blob/v8.x/types/index.d.ts

Sure. I will add this in api.md and index.d.ts.

@PeterAlfredLee
Copy link
Contributor Author

Oh, I have not realized I was having a BREAKING CHANGE in v9 persevere This option should be useful when migrating from v8 to v9 for some users. This should be documented in the API docs (api.md).
And we need to add the typescript type definition as well. https://github.com/kazupon/vue-i18n/blob/v8.x/types/index.d.ts

Sure. I will add this in api.md and index.d.ts.

I added this in api.md, api/README.md and index.d.ts.
I also added the tag :new: 8.26+ in the api.md and api/README.md, but I'm not sure if the 8.26+ description is right or not. Please feel free to tell me if this need to be modified.

PS: Please note that the added description in ru/api/README.md and pt/api/README.md are translated by Google translate.

PPS: And I didn't regenerate the docs cause IMO docs should only be regenerated before a new release.

Copy link
Owner

@kazupon kazupon left a comment

Choose a reason for hiding this comment

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

OK! Thanks!
It looks good to me! :)

@kazupon kazupon added Type: Feature Includes new features and removed PR: reviewed-approved labels Jan 21, 2022
@kazupon kazupon merged commit 8601b2f into kazupon:v8.x Jan 21, 2022
@pr-triage pr-triage bot added the PR: merged label Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged Type: Feature Includes new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants