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: project relative layer locale resolution #2290

Merged
merged 6 commits into from
Aug 11, 2023

Conversation

BobbieGoede
Copy link
Collaborator

@BobbieGoede BobbieGoede commented Aug 2, 2023

πŸ”— Linked issue

#2206

❓ Type of change

These changes fix some unintuitive behaviors we have regarding layer support, it could be seen as a bug fix or feature enhancement.

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

I'm making this PR a draft as I'm not confident whether this PR is non breaking, would appreciate some extra eyes on this. Maybe it's better to delay this until v9? It seems to be working.

At this moment projects are required to do some configuring to be able to make use of extended i18n layers. All locale files are resolved relative to the main project's langDir so the directory has to exist and be defined at i18n.langDir, which in turn requires i18n.locales to contain LocaleObject entries. (see related issue discussion and related PR discussion)

This PR changes the way the locales are resolved by looping through all layers (project and extended) and resolving all locale files relative to the main project, which should make extending i18n layers easier.

I believe this may make some breaking edge cases possible, I still need to test what happens when mixing lazy and non lazy layers or mixing layers with string locales vs LocaleObject locales. The final merged value of lazy is used to treat all provided locales as either lazy or not, this seems to work fine.

Extending a project's i18n config with locales and messages is getting easier and easier, should we think of ways for projects to filter out locales as well?

⚠️ Important note ⚠️
I have added a workaround/hack to be able to properly test our layer implementation, I did this by changing the setup function inside @nuxt/test-utils included in this repo (specs/utils). The changes made to the test-utils can be removed, but we will have to be mindful to use the added overrides property to set project layer configurations.

The @nuxt/test-utils package does not apply nuxtConfig overrides to the project/first layer (nuxt.options._layers[0]), overrides are only applied to the fully merged options. The layer implementation intentionally ignores merged options as files need to be handled/resolved per layer. To work around this I have added an overrides key, essentially implementing our own override approach, which is handled/applied by the module itself. if there is a better way to do this please let me know.

Some tests had to be changed as the layer implementation incorrectly allowed some broken configurations. Also, the gen.test.ts unit tests results are now technically faulty as the langDir locale paths are not resolved inside gen.ts but beforehand.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@BobbieGoede BobbieGoede self-assigned this Aug 2, 2023
@BobbieGoede BobbieGoede force-pushed the feat/layer-locales-resolution branch from b891fc9 to ede2c9b Compare August 5, 2023 08:47
@BobbieGoede BobbieGoede force-pushed the feat/layer-locales-resolution branch from ede2c9b to 39c0075 Compare August 9, 2023 19:38
@BobbieGoede BobbieGoede force-pushed the feat/layer-locales-resolution branch from e764676 to 17500fa Compare August 9, 2023 19:51
@BobbieGoede BobbieGoede marked this pull request as ready for review August 9, 2023 20:03
Copy link
Collaborator

@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.

Thanks!
I got about importance note!
LGTM!

@kazupon kazupon merged commit e33f15b into nuxt-modules:next Aug 11, 2023
4 checks passed
@dargmuesli
Copy link
Collaborator

dargmuesli commented Aug 16, 2023

Does this change anything about the content I edited in #2268? Like, do the docs need an update?

@BobbieGoede
Copy link
Collaborator Author

@dargmuesli
It does, I should have included it in this PR as it's not necessary to configure any options to extend on an i18n layer now. I will open a PR to fix it! πŸ˜…

DarthGigi pushed a commit to DarthGigi/i18n that referenced this pull request Apr 16, 2024
* test: add `overrides` property for overriding project layer options in tests

* feat: relative locale resolution

* feat: validate layer options

* test: fix incorrect test fixtures

* fix: use defu to merge inline options

* refactor: remove unused `langDir`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants