feat: Add NO_PREFIX strategy + setLocale function #409

richie3366 · 2019-08-22T16:45:14Z

For those who only want to use SEO, lazy locale load, cookie store and/or browser-language detection without having to deal with sub-domains nor route prefixes nor route aliases, the no_prefix Strategy will allow nuxt-i18n to fit their needs.

I've also added a setLazyLocale function in app.i18n to let developers call it programatically.

codecov · 2019-08-22T16:46:38Z

Codecov Report

Merging #409 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #409      +/-   ##
==========================================
- Coverage    81.1%   81.03%   -0.07%     
==========================================
  Files           4        3       -1     
  Lines         127      116      -11     
  Branches       23       26       +3     
==========================================
- Hits          103       94       -9     
+ Misses         20       18       -2     
  Partials        4        4

Impacted Files	Coverage Δ
src/index.js	`86.04% <100%> (+2.26%)`	⬆️

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 63ed8df...996b7ae. Read the comment docs.

rchl · 2019-08-22T20:11:45Z

I think we should have a warning (console.warn) in localePath function that would trigger when locale argument is different than current locale. Basically there is no way to handle that with no-prefix strategy so we should warn that passing locale is unsupported in that case.
Same for switchLocalePath - it won't work as expected when passed non-default locale.
I believe the differentDomains option is not compatible with no-prefix strategy. One will break functionality of the other if both are set. Would be nice to have warning for that too (in module's index.js).

rchl

You are a godsend. I needed that and was going to look into it myself soon. :)

src/helpers/routes.js

src/plugins/main.js

src/plugins/routing.js

rchl · 2019-08-22T20:39:06Z

Also this code should be disabled with no-prefix strategy:
https://github.com/nuxt-community/nuxt-i18n/blob/ff6937bf8bebd3d98d51abf2098046b5323b13f7/src/templates/middleware.js#L102-L104
It would also be problematic if you would follow my advice and trigger warning when calling localePath with non-default locale argument.

rchl · 2019-08-22T20:57:10Z

Please also add two new APIs here:
https://github.com/nuxt-community/nuxt-i18n/blob/ff6937bf8bebd3d98d51abf2098046b5323b13f7/types/vue.d.ts#L45-L54

richie3366 · 2019-08-23T07:25:53Z

Thanks for your appreciation & for all your comments @rchl!

Some of the things you noticed were caused by the fact I had implemented them 6 months ago (without making a PR because I thought it may be undesired since it's kinda the core/main functionnality of this module) in a different context, and I applied the same changes without checking if it could have a better fit.

I'll look further into your comments during the day (CEST timezone) and I'll do my best to answer &/or comply to them.

richie3366 · 2019-08-23T12:51:06Z

@rchl: I think I've made all the requested changes. Do you advise to rewrite my commit (& force push) or to create an additional one?

rchl · 2019-08-23T12:53:29Z

Feel free to add new commits. It's easier to review that way. I can rewrite stuff when merging. Thanks

richie3366

I think we should have a warning (console.warn) in localePath function that would trigger when locale argument is different than current locale. Basically there is no way to handle that with no-prefix strategy so we should warn that passing locale is unsupported in that case.

Same for switchLocalePath - it won't work as expected when passed non-default locale.

I believe the differentDomains option is not compatible with no-prefix strategy. One will break functionality of the other if both are set. Would be nice to have warning for that too (in module's index.js).

I'm getting a lot of warnings* at init & every time I navigate. Maybe I did it wrong? Or maybe we simply shouldn't interfere at all with localePath & swithLocalePath? Or not calling them in the lib?

* not getting the differentDomains warn though

rchl · 2019-08-23T13:11:47Z

Can't comment without seeing the code but my idea for warnings was to trigger them only when locale is passed AND it doesn't match current locale. Is that how you did it?

richie3366 · 2019-08-23T13:19:40Z

Can't comment without seeing the code but my idea for warnings was to trigger them only when locale is passed AND it doesn't match current locale. Is that how you did it?

I compared the values like you said:

localePath: when locale differs from current locale:

    if (!locale) return // <- was already there

    if(STRATEGY === STRATEGIES.NO_PREFIX && locale !== this.$i18n.locale) {
      console.warn('[<%= options.MODULE_NAME %>] localePath is unsupported when using no_prefix strategy', locale, this.$i18n.locale)
    }

switchLocalePath: when locale differs from default locale:

    if(STRATEGY === STRATEGIES.NO_PREFIX && locale && locale !== defaultLocale) {
      console.warn('[<%= options.MODULE_NAME %>] switchLocalePath is unsupported when using no_prefix strategy', locale, defaultLocale)
    }

This is what I have in my console:

rchl · 2019-08-23T13:21:54Z

Ok. Feel free to either investigate more or leave it out. I can look later.

richie3366 · 2019-08-23T14:04:56Z

Ok. Feel free to either investigate more or leave it out. I can look later.

You seem to have a good knowledge about the mechs/code of this lib, I think you'll find the fix quicker
(and maybe cleaner) than me

richie3366 · 2019-08-23T14:08:44Z

Whoops, forgot to run the tests before committing, something broke in between, sorry for that.

src/helpers/routes.js

src/index.js

src/plugins/routing.js

types/vue.d.ts

HapLifeMan · 2019-08-23T20:51:25Z

The majority of your request changes are problems that would not occur with a stricter ESLint config, what do you think about complete the .eslintrc.js file @rchl? That would improve a lot every contribution to this repo and save a lot of time for each contributor and reviewer. 🙂

rchl · 2019-08-23T22:18:27Z

@HapLifeMan If you can figure out a config that would work in files that contain lodash templates then I'm all ears. I've failed before. :)

rchl · 2019-08-24T10:22:42Z

As for the warnings, it's due to seo-head.js adding alternate links for all locales. That code needs to be disabled for NO_PREFIX locale as we have no alternate links then. I think it's OK to keep the code that adds og:locale:alternate.

src/plugins/main.js

HapLifeMan · 2019-08-24T12:38:54Z

I've updated files according to your last request changes!

About the ESLint, we have a very strict ESLint config where we are working and this is not my repo so I wouldn't want to oblige contributors to follow our personal ESLint rules without your permission.
But I'm pretty sure you can work with this plugin. If you're open to ESLint config suggestions that may involve a lot of syntax changes, let me know and I'll be happy to share it here 🙂

rchl · 2019-08-24T12:50:23Z

This repo is already using pretty strict rules that would catch all the issues I've pointed out. Problem is that all files with templates are ignored. I've looked at that plugin before and had some problems but maybe those problems don't apply here. I'll check again later.

Will look at your changes a bit later when I'm home. Thanks :)

HapLifeMan · 2019-08-24T15:12:50Z

You're welcome, I said that because we have a 60 rules ESLint config 😉

rchl · 2019-08-24T15:25:07Z

@HapLifeMan this project is extending this config: https://github.com/standard/eslint-config-standard/blob/master/eslintrc.json
So it has quite a bit of rules. :)

src/plugins/routing.js

HapLifeMan · 2019-08-24T15:57:13Z

Updated! Oh I didn't saw the extends, sorry about that 🙈

rchl · 2019-08-24T18:01:52Z

I'm now happy with those changes.

Some remaining things that can be done separately:

problem I've mentioned in one of the comments:

There is also an issue with this API (setLocale) when using other than no-prefix strategies. It will change locale but route URL will stay unchanged (with previous locale prefix). I'm not sure what's the best way to handle that - two options I see are: call nuxt's redirect or replace route using VueRouter.replace(). Both can be destructive but I guess that should be OK... I can handle that myself later.

improving test coverage. Testing needs some cleanup...

found new problem

rchl

Found some more problems while starting to look into tests:

~~setLocale API only works with lazy loading enabled. I'll have a look at that.~~ FIXED
Generation of routes actually does something that should in theory also be done for no_prefix strategy - it generates routes with different name if user has specified name override. It would be problematic to make that work as we would need to then apply proper locale for that route regardless of what is currently used. I don't think there is enough context anywhere to figure that out... Maybe this should not be supported for now as it would require quite a lot of work probably...

rchl · 2019-08-26T09:12:17Z

Status:

created Investigate linting files with templates #415 for improving linting but as you can see there, I'm not having too much luck. @HapLifeMan
I'm waiting for feat: allow specifying merge/overwrite strategy loadConfig nuxt/test-utils#2 before I'll progress further here

richie3366 · 2019-08-26T12:27:53Z

HapLifeMan's commit : await was needed to fix a bug in our project, reason: vue-i18n was loading before nuxt-i18n had finished to init & load the default locale

My recent commits: cf. commit descriptions

Forgot to mention: my first commit of the day also implemented the auto-redirect to localePath when using setLocale programatically :)

src/plugins/main.js

rchl · 2019-08-26T13:52:33Z

HapLifeMan's commit : await was needed to fix a bug in our project, reason: vue-i18n was loading before nuxt-i18n had finished to init & load the default locale

Clearly a right thing to do. My bad for forgetting it.

rchl

I'll keep it unapproved for now as I'm still blocked with tests.

src/plugins/main.js

richie3366 · 2019-08-26T15:05:43Z

I'll keep it unapproved for now as I'm still blocked with tests.

Okay, if you need anything else from me, feel free to ask!

rchl

I'm happy with this now. :)

Added a bugfix to wrongly stringified STRATEGIES, added tests and did some small refactorings.

Will rebase and merge this tomorrow after you agree that it's ready. :)

Thank you both for your work.

paulgv

Thank you so much for working on this @richie3366! This will be an awesome improvement!

paulgv · 2019-08-26T21:16:06Z

src/index.js

@@ -39,6 +39,12 @@ module.exports = function (userOptions) {
    }
  }

+  if (!Object.values(STRATEGIES).includes(options.strategy)) {
+    // eslint-disable-next-line no-console
+    console.error('[' + options.MODULE_NAME + '] Invalid "strategy" option "' + options.strategy + '" (must be one of: ' + Object.values(STRATEGIES).join(', ') + ').')


Thanks for adding this check!

HapLifeMan · 2019-08-26T22:53:02Z

Will rebase and merge this tomorrow after you agree that it's ready. :)
Thank you both for your work.

Thank you for your suggestions and improvements! This is really a good thing to have an active community open to new features like that. You can rebase & merge 😄

Also: - chore: remove constants.js from coverage The coverage was patching the default beforeLanguageSwitch & onLanguageSwitched null functions, but tests failed because the patched functions referenced unknown (out of scope?) variables. + I think that making coverage on a constant-declarations file is kinda pointless since all the lines are always covered by design. - fix(tests): don't use v-for and v-if together - warning fix - refactor: hoist constants outside of exported function - refactor: don't return messages from loadLanguageAsync Co-authored-by: Thomas Reichling <thomas.reichling@gmail.com> Co-authored-by: Rafał Chłodnicki <rchl2k@gmail.com>

paulgv · 2019-08-27T22:39:30Z

It's a bit late for me to notice this, but it looks like this PR might have broken something in the module, my first guess would be that it's related to the code that was removed from the middleware.

To reproduce the bug I'm seeing:

Run yarn dev:basic in the module's directory
Visit http://localhost:3000/
Click on About us
Click on Français to switch locales

I would expect to end up on http://localhost:3000/fr/a-propos but I'm actually redirected to http://localhost:3000/fr. The lang switcher's link itself is correct, so I assume that something in the middleware overrides the route and redirects to the homepage for whatever reason.

@richie3366 @rchl WDYT? Is there any way we can fix this quickly? Should we revert these changes for the time being?

rchl · 2019-08-28T05:10:39Z

@paulgv have you tried fix in #418?

rchl · 2019-08-28T07:24:08Z

Actually I can still reproduce with the fix. Will look more into it.

Let's move this issue over to the #418 PR.

rchl requested changes Aug 22, 2019

View reviewed changes

richie3366 commented Aug 23, 2019

View reviewed changes

rchl requested changes Aug 23, 2019

View reviewed changes

rchl requested changes Aug 24, 2019

View reviewed changes

src/plugins/main.js Outdated Show resolved Hide resolved

rchl requested changes Aug 24, 2019

View reviewed changes

src/plugins/routing.js Outdated Show resolved Hide resolved

src/plugins/routing.js Outdated Show resolved Hide resolved

rchl requested changes Aug 24, 2019

View reviewed changes

src/plugins/routing.js Outdated Show resolved Hide resolved

src/plugins/routing.js Outdated Show resolved Hide resolved

rchl previously approved these changes Aug 24, 2019

View reviewed changes

rchl self-requested a review August 24, 2019 20:50

rchl requested changes Aug 24, 2019

View reviewed changes

rchl pushed a commit to Pickaw/nuxt-i18n that referenced this pull request Aug 24, 2019

feat: Add NO_PREFIX strategy + setLocale function (nuxt-modules#409)

0cda3ef

rchl force-pushed the new-no-prefix branch from 17a8b1b to 0cda3ef Compare August 24, 2019 20:55

rchl pushed a commit to Pickaw/nuxt-i18n that referenced this pull request Aug 24, 2019

feat: Add NO_PREFIX strategy + setLocale function (nuxt-modules#409)

12812c2

rchl force-pushed the new-no-prefix branch from 0cda3ef to 12812c2 Compare August 24, 2019 21:13

rchl requested changes Aug 26, 2019

View reviewed changes

rchl closed this Aug 26, 2019

rchl reopened this Aug 26, 2019

rchl requested changes Aug 26, 2019

View reviewed changes

src/plugins/main.js Show resolved Hide resolved

rchl changed the title ~~feat: Add NO_PREFIX strategy + setLazyLocale function~~ feat: Add NO_PREFIX strategy + setLocale function Aug 26, 2019

rchl pushed a commit to Pickaw/nuxt-i18n that referenced this pull request Aug 26, 2019

feat: Add NO_PREFIX strategy + setLocale function (nuxt-modules#409)

a70e04b

rchl force-pushed the new-no-prefix branch 2 times, most recently from e8c5828 to 34f264e Compare August 26, 2019 18:35

rchl self-requested a review August 26, 2019 21:02

rchl force-pushed the new-no-prefix branch from e320188 to b440d43 Compare August 26, 2019 21:14

rchl approved these changes Aug 26, 2019

View reviewed changes

paulgv reviewed Aug 26, 2019

View reviewed changes

rchl force-pushed the new-no-prefix branch from 0f114f4 to 996b7ae Compare August 27, 2019 09:15

rchl merged commit 998011e into nuxt-modules:master Aug 27, 2019

feat: Add NO_PREFIX strategy + setLocale function #409

feat: Add NO_PREFIX strategy + setLocale function #409

Conversation

richie3366 commented Aug 22, 2019

codecov bot commented Aug 22, 2019 • edited

Codecov Report

rchl commented Aug 22, 2019 • edited

rchl left a comment

Choose a reason for hiding this comment

rchl commented Aug 22, 2019 • edited

rchl commented Aug 22, 2019

richie3366 commented Aug 23, 2019

richie3366 commented Aug 23, 2019

rchl commented Aug 23, 2019 • edited

richie3366 left a comment • edited

Choose a reason for hiding this comment

rchl commented Aug 23, 2019

richie3366 commented Aug 23, 2019

rchl commented Aug 23, 2019

richie3366 commented Aug 23, 2019

richie3366 commented Aug 23, 2019

HapLifeMan commented Aug 23, 2019

rchl commented Aug 23, 2019

rchl commented Aug 24, 2019 • edited

HapLifeMan commented Aug 24, 2019

rchl commented Aug 24, 2019

HapLifeMan commented Aug 24, 2019

rchl commented Aug 24, 2019

HapLifeMan commented Aug 24, 2019

rchl commented Aug 24, 2019 • edited

rchl left a comment • edited

Choose a reason for hiding this comment

rchl commented Aug 26, 2019 • edited

richie3366 commented Aug 26, 2019 • edited

rchl commented Aug 26, 2019

rchl left a comment

Choose a reason for hiding this comment

richie3366 commented Aug 26, 2019

rchl left a comment • edited

Choose a reason for hiding this comment

paulgv left a comment

Choose a reason for hiding this comment

paulgv Aug 26, 2019

Choose a reason for hiding this comment

HapLifeMan commented Aug 26, 2019

paulgv commented Aug 27, 2019

rchl commented Aug 28, 2019

rchl commented Aug 28, 2019 • edited

codecov bot commented Aug 22, 2019 •

edited

rchl commented Aug 22, 2019 •

edited

rchl commented Aug 22, 2019 •

edited

rchl commented Aug 23, 2019 •

edited

richie3366 left a comment •

edited

rchl commented Aug 24, 2019 •

edited

rchl commented Aug 24, 2019 •

edited

rchl left a comment •

edited

rchl commented Aug 26, 2019 •

edited

richie3366 commented Aug 26, 2019 •

edited

rchl left a comment •

edited

rchl commented Aug 28, 2019 •

edited