-
-
Notifications
You must be signed in to change notification settings - Fork 760
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 public directory support #525
Conversation
…ecation of static directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll also need test coverage for the console warning.
@@ -20,4 +20,5 @@ const localeSubpathVariations = { | |||
module.exports = new NextI18Next({ | |||
otherLanguages: ['de'], | |||
localeSubpaths: localeSubpathVariations[localeSubpaths], | |||
localePath: typeof window === "undefined" ? "public/locales" : "static/locales", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behaviour should be built in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is a question or affirmation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An affirmation - it doesn't make sense to require all users to perform the same check.
const fs = eval("require('fs')") | ||
const path = require('path') | ||
const hasStaticDir = fs.existsSync(path.join(process.cwd(), 'static')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be checking if there are namespace files here, not just if the dir exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be on the same line or adding another validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it could be another const
called hasDefaultNamespace
or something.
const hasStaticDir = fs.existsSync(path.join(process.cwd(), 'static')) | ||
const hasPublicDir = fs.existsSync(path.join(process.cwd(), 'public')) | ||
|
||
let staticLegacy = 'static/locales' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work. We need to respect the localePath
in case a user has provided a non-default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, so we just add the warning and the localePath
change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure exactly what you mean.
@isaachinman These days I could send the PR update, but I see that there is an open PR. I have no problem updating or continuing with the new. |
Closing in favour of #569. |
close: #523
notes
Add support to the example for the tests to be successful