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 - compile time inlining #32881
I18n - compile time inlining #32881
Conversation
13aa1f1
to
97aee88
Compare
50f4f23
to
5332292
Compare
e6b796d
to
39559c9
Compare
@petebacondarwin FYI I'm in the process of reviewing this PR, will let you know once it's completed (most likely Monday, 10/7). Thank you. |
Thanks Andrew. I am going to tweak the API a bit, today, after discussions with @alan-agius4. But I will add the changes as fixup commits so that it doesn't affect your review if you are going commit by commit. |
ee238ab
to
6ca46a9
Compare
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.
Very impressive work @petebacondarwin! 👍
I left a few minor comments, none of them are blockers.
Thank you.
packages/localize/src/tools/src/translate/source_files/source_file_utils.ts
Outdated
Show resolved
Hide resolved
packages/localize/src/tools/src/translate/source_files/source_file_utils.ts
Outdated
Show resolved
Hide resolved
packages/localize/src/tools/src/translate/source_files/source_file_utils.ts
Outdated
Show resolved
Hide resolved
...ize/src/tools/src/translate/translation_files/translation_parsers/translation_parse_error.ts
Show resolved
Hide resolved
...ools/src/translate/translation_files/translation_parsers/xliff1/xliff1_message_serializer.ts
Show resolved
Hide resolved
d4d462f
to
4e73f23
Compare
FYI, VE and Ivy presubmits are successful. Thank you. |
4e73f23
to
fd03a31
Compare
This commit implements a tool that will inline translations and generate a translated copy of a set of application files from a set of translation files.
fd03a31
to
19b6d96
Compare
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.
I did a high level review of the PR (general architecture, public api, docs, tests) but didn't check all the details - I'm relying on other reviewers for that.
everything looks good, except for the missing license for babel__core types. Can you please fix that - you can create a new license.md file if one doesn't exist in the original repo, as long as you populate it with the correct licensing info. thanks!
const templateBuilder: DefaultTemplateBuilder; | ||
|
||
export default templateBuilder; | ||
} |
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.
can you please ensure that these typings files don't end up in the built npm packages?
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.
I'm not sure how to configure the Bazel rules to do that...
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.
I'm not either, unfortunately. That would be a good @alexeagle question. I don't know how npm_package
determines the set of files to include.
Do references to these types end up in the .d.ts
files for the package? If so, it might be very difficult to get the package build to exclude them.
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.
Instead I suggest that we just try to get bazelbuild/rules_nodejs#1033 fixed, which will allow us to remove these typings files altogether.
How about we land this PR now and then work to fix that issue and remove the typings before RC?
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.
Per Igor, this is the approach we'll take. I will merge this PR soon.
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.
@petebacondarwin Is there a Jira for this? Who is working on the Bazel fixes?
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.
@alexeagle is working on the fix as part of the Bazel team. I don't know if there is a JIRA for it. There is not one in the FW JIRA.
hmm.. and ci seems to be failing due to duplicate dependencies? @petebacondarwin can you please take a look? |
19b6d96
to
79d7de2
Compare
79d7de2
to
c9e3a8a
Compare
@IgorMinar - @babel/core typings LICENSE file added |
This commit implements a tool that will inline translations and generate a translated copy of a set of application files from a set of translation files. PR Close angular#32881
…ngular#32881) This closer reflects what caused the error. PR Close angular#32881
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.