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

Allow function type for localePath #1593

Closed
wants to merge 4 commits into from

Conversation

lucaslcode
Copy link
Contributor

@lucaslcode lucaslcode commented Dec 22, 2021

Fixes #1500

localePath as a function is of the form (locale: string, namespace: string, env: 'client' | 'server', missing: boolean) => string returning the entire path including filename and extension. When missing is true, return the path for the addPath option of i18next-fs-backend, when false, return the path for the loadPath option. More info at the i18next-fs-backend repo.

See comments

Tests added Wasn't sure how to add tests for the additional functionality - any guidance appreciated (maybe I should add tests for the error cases and that the function is correctly in the final config by calling it?)

@vercel
Copy link

vercel bot commented Dec 22, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/isaachinman/next-i18next/26jw96b8bTAGiXYCFLEu3sdVAMRz
✅ Preview: https://next-i18next-git-fork-couchers-org-function-943f13-isaachinman.vercel.app

if (localePath.match(/^\.?\/public\//)) {
clientLocalePath = localePath.replace(/^\.?\/public/, '')
}
const clientLocalePath = typeof localePath === 'string' && localePath.match(/^\.?\/public\//)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Has to be const to satisfy typescript, since line 157 uses a closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally, it seems to me this stripping of /public on the client isn't necessary, since nothing is even read on the client - I'm not totally sure though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we can probably remove the stripping of /public. It's a remnant of when this package used to use i18next-http-backend by default, and used getInitialProps, necessitating client side loading of namespaces.

@isaachinman
Copy link
Contributor

Thanks @lucaslcode! Just wanted to give a heads up that I am travelling now, and probably won't be able to review this until sometime in January. Rest assured I will work with you to get this merged as soon as possible.

@lucaslcode
Copy link
Contributor Author

lucaslcode commented Dec 28, 2021 via email

Copy link
Contributor

@isaachinman isaachinman left a comment

Choose a reason for hiding this comment

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

Thanks for your patience – finally had a chance to get to this.

Just a few comments to discuss a bit, but overall I am extremely impressed with both the quality of the code, and your ability to understand the wider repo.

Massive thanks for this contribution!

src/config/createConfig.ts Outdated Show resolved Hide resolved
src/config/createConfig.ts Outdated Show resolved Hide resolved
src/config/createConfig.ts Outdated Show resolved Hide resolved
if (localePath.match(/^\.?\/public\//)) {
clientLocalePath = localePath.replace(/^\.?\/public/, '')
}
const clientLocalePath = typeof localePath === 'string' && localePath.match(/^\.?\/public\//)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we can probably remove the stripping of /public. It's a remnant of when this package used to use i18next-http-backend by default, and used getInitialProps, necessitating client side loading of namespaces.

src/serverSideTranslations.ts Outdated Show resolved Hide resolved
@lucaslcode
Copy link
Contributor Author

@isaachinman thanks for the review (and the compliment 😄). I've addressed the feedback, including removing the stripping of /public and therefore no longer including env: 'server' | 'client' in the callback parameters, since there isn't a use-case for it as far as I can tell.

README.md Outdated Show resolved Hide resolved
@isaachinman
Copy link
Contributor

Looks great @lucaslcode, thanks for all your help!

Weirdly this PR is not running tests in Circle – I think it's related to a CircleCI OAuth issue a couple weeks ago. Can you try rebasing against latest master? If that doesn't work, we may need to recreate the PR entirely.

No longer strips the '/public' directory in the client,
so removed the 'env' parameter from localePath callback.
@lucaslcode
Copy link
Contributor Author

I'll try a new PR

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.

localeStructure without sub-directory keeps searching for subdirectory
2 participants