-
Notifications
You must be signed in to change notification settings - Fork 592
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
I18n fix #4294
Conversation
PRO-4701 Ignore dotfiles in `i18n` folder during build because Apostrophe breaks for Mac users who try to use the finder to access folders.
During a project build, an error will occur if there is an If there is any file in there, it will cause an error. |
modules/@apostrophecms/i18n/index.js
Outdated
@@ -523,7 +523,15 @@ module.exports = { | |||
self.namespaces[ns].browser = self.namespaces[ns].browser || | |||
(metadata[ns] && metadata[ns].browser); | |||
const namespaceDir = path.join(localizationsDir, ns); | |||
if (!fs.statSync(namespaceDir).isDirectory()) { | |||
// Handle files in the namespace that aren't JSON files |
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.
Checking to make sure it's a directory makes sense, but that seems unconnected to the stated purpose in the comment, except in a limited sense.
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.
Currently, the conditional at the top of for
loop checks if the current item is a JSON file and if it is skips it because it has been handled by the addDefaultResourcesForModule()
method. At this point, the method is assuming that everything else is a directory, which isn't always true. Therefore, we need to a check to jump to the next item in the loop if it is a file (and since we already checked for JSON files, it would be a non-JSON file.. This would include .DS_Store
that is breaking the build. I'll change the comment.
But, maybe I'm not understanding your concern.
modules/@apostrophecms/i18n/index.js
Outdated
for (const localizationFile of fs.readdirSync(namespaceDir)) { | ||
if (!localizationFile.endsWith('.json')) { | ||
// A JSON file for the default namespace, already handled |
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.
How would a non-json file in a namespace folder be a JSON file for the default namespace? This check makes sense but the comment has me wondering if things were transposed somehow.
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.
Yeah - I think that copilot "helpfully" wrote that comment and I wasn't careful enough in reading it. That conditional makes sure that within the namespace folder only JSON files are being parsed. I'll change the comment.
Summary
Summarize the changes briefly, including which issue/ticket this resolves. If it closes an existing Github issue, include "Closes #[issue number]"
Currently, if an i18n folder has any files inside besides JSON translation string files, it will break the build process. For example, this can occur if a user creates a new JSON file using the Mac OS finder. The finder will leave behind a
.DS_Store
file. This PR fixes this issue and closes PRO-4701.What are the specific steps to test this change?
For example:
modules/<some-module>/i18n/test.txt
andmodules/<some-module>/i18n/testNs/test.txt
. The first will test without namespace, the second with. Testbed already has the structure in place for thetopic
module.npm run dev
What kind of change does this PR introduce?
(Check at least one)
Make sure the PR fulfills these requirements:
If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.
Other information: