-
Notifications
You must be signed in to change notification settings - Fork 9.6k
i18n: fix import scripts #9621
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 import scripts #9621
Conversation
brendankenny
left a comment
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.
LGTM w/ request(s)
| for (const localePath of localePaths) { | ||
| const absoluteLocalePath = path.join(lhRoot, localePath); | ||
| const localeLhl = require(absoluteLocalePath); | ||
| const localeLhl = JSON.parse(fs.readFileSync(absoluteLocalePath, 'utf-8')); |
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.
Change seems fine (and probably for the best, who knows where we'll run pruning from in the future) but just to clarify, where else do we require() the lhl files in collect-strings and how does the error manifest?
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 we get the comment in here about why we needed to do this? it certainly wasn't obvious to me debugging that it's run in the same process as collect-strings :)
and the error manifests in the update command being a noop because we use the pre-overriden strings instead of the new ones
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 certainly wasn't obvious to me debugging that it's run in the same process as collect-strings :)
this may be one of those things that's obvious while writing it but isn't when running it, so we could change it. The idea was that collect-strings collects then creates an updated en-US.json, so it made sense at that point to make sure none of the existing LHL files conflicted with the new en-US.json.
and the error manifests in the update command being a noop because we use the pre-overriden strings instead of the new ones
what action is being done that's a no-op? #9598 definitely removed strings after running collect-strings.js. What else in there is loading the locale json files and overwriting them with the cached version?
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.
what action is being done that's a no-op?
the entire script command that updates the locales from tc becomes a no-op because this function just overwrites them with the locales that were there originally (i.e. it "prunes" the cached copy loaded require which is old)
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.
the entire script command that updates the locales from tc becomes a no-op because this function just overwrites them with the locales that were there originally
ah, see @exterkamp, this is why that script should be in this repo :P
patrickhulce
left a comment
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.
LGTM
| for (const localePath of localePaths) { | ||
| const absoluteLocalePath = path.join(lhRoot, localePath); | ||
| const localeLhl = require(absoluteLocalePath); | ||
| const localeLhl = JSON.parse(fs.readFileSync(absoluteLocalePath, 'utf-8')); |
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 we get the comment in here about why we needed to do this? it certainly wasn't obvious to me debugging that it's run in the same process as collect-strings :)
and the error manifests in the update command being a noop because we use the pre-overriden strings instead of the new ones
Summary
This fixes 2 issues:
requirewhich does not reload a file if it is modified in the same process, which is what happens when youcollect-strings. So we need to reload it to pick up changes in the lhl from an import. (thanks @patrickhulce)