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: Dynamic route parameters translation #345

Merged
merged 5 commits into from
Jul 13, 2019
Merged

Conversation

paulgv
Copy link
Collaborator

@paulgv paulgv commented Jul 1, 2019

What's new?

Adds support for translating dynamic route parameters via the Vuex store module

BREAKING CHANGE: preserveState is now set automatically when registering the store module and
cannot be set via the configuration anymore

So why do we need this?

When using switchLocalePath to create a global lang switcher component, there's currently no way for the method to know how to translate dynamic parameters. This is an attempt to address this issue.

How to test this?

This has been published as a prerelease on NPM, to test this version, install v6.0.0-0 with Yarn:

Or NPM:

Refer to the new documentation section to see how to use this in your app: Dynamic route parameters

close #79

@paulgv paulgv self-assigned this Jul 1, 2019
@codecov
Copy link

codecov bot commented Jul 1, 2019

Codecov Report

Merging #345 into 6.x will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##            6.x   #345   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         2      2           
  Lines         6      6           
===================================
  Hits          6      6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 918ae89...cce3774. Read the comment docs.

@paulgv paulgv force-pushed the dynamic-params-translations branch 2 times, most recently from e4033ca to 04373ef Compare July 1, 2019 15:11
@paulgv paulgv requested a review from rchl July 1, 2019 15:27
@paulgv
Copy link
Collaborator Author

paulgv commented Jul 1, 2019

@rchl If you get the chance to have a look at this, please let me know what you think.

Copy link
Collaborator

@rchl rchl left a comment

Choose a reason for hiding this comment

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

Thank's for for reaching out to me to get my opinion.

I'm gonna keep looking more and thinking about it but first thing that comes to my mind is that I don't like the fact that this feature is dependant on store. I have personally never used i18n store as I didn't see the point of it. All it seems to do for me is duplicating all translation strings
that are already available in bundles, making page slower to load. So I would prefer that this feature doesn't require one to enable store. From the top of my head, I don't have better solution but maybe we can think of something :).

docs/lang-switcher.md Outdated Show resolved Hide resolved
test/fixtures/basic/pages/posts/_slug.vue Outdated Show resolved Hide resolved
@rchl
Copy link
Collaborator

rchl commented Jul 1, 2019

Wouldn't it be simpler if switchLocalePath accepted an optional object with that information that is currently passed through store?

@rchl
Copy link
Collaborator

rchl commented Jul 1, 2019

Alternatively, I wonder if completely declarative approach is possible. That is, extract that information from pages at built time and maybe use Route object to store that information (Route.meta maybe)...

@paulgv
Copy link
Collaborator Author

paulgv commented Jul 1, 2019

Thanks for the feedback @rchl

Wouldn't it be simpler if switchLocalePath accepted an optional object

I think that users typically don't want to call switchLocalePath more than once in the app. It's pretty easy to create a LangSwitcher component and render it in the layout without having to think about special cases like the one we're trying to address here. I feel like pages need to be able provide parameters translations seamlessly and let switchLocalePath deal with these parameters "backstage".

extract that information from pages at built time

Since we're dealing with dynamic paths here, we can't really get the required data at build time because it might result in huge payloads in router meta if we were to handle all possible parameters & translations for all dynamic routes. Additionally, since the data will most likely grow between each build, the router will frequently be out-of-sync with the database.

I would prefer that this feature doesn't require one to enable store

Agreed. I ended up going for this solution even though I'm not a fan of it because I can't seem to make anything else work properly. Ideally, I'd like nuxt-i18n to expose a custom method trough pages components $options, that would return translated route parameters and the module would handle the rest. But we still need to make that data accessible globally for switchLocalePath to have access to it wherever it's used, while still being consistent on both the server and the client-side, which always a feels a bit tricky to me. I tried another solution where I injected a global Vue.observable that would serve as single purpose store but its state was lost on the client-side and I couldn't figure out why.

Any thoughts?

@paulgv
Copy link
Collaborator Author

paulgv commented Jul 1, 2019

Wouldn't it be simpler if switchLocalePath accepted an optional object

To clarify my thoughts here. I think that your suggestion would absolutely be valid and we probably should implement it anyway. But if I'm not mistaken, it's not compatible with using a "global" lang switcher in the layout, it would require users to render the lang switcher in every page, specifying translated parameters where needed, which could get a bit tedious in large applications.

@rchl
Copy link
Collaborator

rchl commented Jul 2, 2019

Good point about having language switcher in the layout. Then that wouldn't work if data would be returned from page's asyncData. Didn't think of that.

In that case I guess we should go for current solution. I wonder though, given that it will be a major version, should we disable setLocale and setMessages mutations by default? I feel like it's something that not many people need but it can add a lot of heft and users might not even realize that.

Copy link
Collaborator

@rchl rchl left a comment

Choose a reason for hiding this comment

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

Seems OK to me. Feel free to either address my comments or not. :)

@paulgv
Copy link
Collaborator Author

paulgv commented Jul 2, 2019

Thanks for the great suggestions @rchl, I'll address them ASAP.

@paulgv paulgv changed the base branch from master to 6.x July 4, 2019 12:42
@paulgv paulgv force-pushed the dynamic-params-translations branch from 6781612 to 17f17d8 Compare July 4, 2019 12:42
Adds support for translating dynamic route parameters via the Vuex store module

BREAKING CHANGE: `preserveState` is now set automatically when registering the store module and
cannot be set via the configuration anymore

close #79
@paulgv paulgv force-pushed the dynamic-params-translations branch from 17f17d8 to b8f638b Compare July 13, 2019 12:14
@paulgv paulgv merged commit c60a618 into 6.x Jul 13, 2019
@paulgv paulgv deleted the dynamic-params-translations branch July 13, 2019 13:55
paulgv added a commit that referenced this pull request Jul 15, 2019
* feat: Dynamic route parameters translation

Adds support for translating dynamic route parameters via the Vuex store module

BREAKING CHANGE: `preserveState` is now set automatically when registering the store module and
cannot be set via the configuration anymore

close #79
paulgv added a commit that referenced this pull request Jul 20, 2019
* feat: Dynamic route parameters translation

Adds support for translating dynamic route parameters via the Vuex store module

BREAKING CHANGE: `preserveState` is now set automatically when registering the store module and
cannot be set via the configuration anymore

close #79
paulgv added a commit that referenced this pull request Jul 20, 2019
* feat: Dynamic route parameters translation

Adds support for translating dynamic route parameters via the Vuex store module

BREAKING CHANGE: `preserveState` is now set automatically when registering the store module and
cannot be set via the configuration anymore

close #79
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.

2 participants