Skip to content

refactor(compiler-cli): remove i18n options from LinkerOptions #41554

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

Conversation

petebacondarwin
Copy link
Contributor

There were three options being made available to users of the linker:

  • enableI18nLegacyMessageIdFormat
  • i18nNormalizeLineEndingsInICUs
  • i18nUseExternalIds

None of these should actually be configurable at linking time
because partially-linked libraries have tighter restrictions on
what i18n options can be used.

This commit removes those options from the LinkerOptions interface.
It was considered to add a check for backwards compatibilty to ensure
that if these options were being passed, and were different to the expected
defaults, we would throw an informative error. But from looking at the
Angular CLI (the only known client of the linker) it has never been setting
these options so they have already always been set to the defaults.

BREAKING CHANGE:

Linked libraries no longer generate legacy i18n message ids. Any downstream
application that provides translations for these messages, will need to
migrate their message ids using the localize-migrate command line tool.

Closes #40673

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer refactoring Issue that involves refactoring or code-cleanup breaking changes target: major This PR is targeted for the next major release compiler: linker labels Apr 10, 2021
@google-cla google-cla bot added the cla: yes label Apr 10, 2021
@pullapprove pullapprove bot requested a review from alxhub April 10, 2021 12:56
@petebacondarwin petebacondarwin requested a review from JoostK April 10, 2021 12:58
@pullapprove pullapprove bot requested a review from devversion April 10, 2021 14:31
@petebacondarwin petebacondarwin force-pushed the linker-remove-i18n-options branch from 4eab2e0 to 610b29d Compare April 10, 2021 17:12
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

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

LGTM. I am not sure about the deprecation notice, given that partial compilation/the linker has not been stable so in and of itself I wouldn't consider this change as breaking. It is however true that a library that switches to partial compilation would indeed be breaking, as the application's compilation options are no longer considered (so no more legacy message ids nor is preserveWhitespaces taken into account anymore). I notice you put this on the meeting agenda so let's discuss there. In any case, I think the deprecation notice can not go into a refactor commit as those are excluded from the changelog.

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM for dev infra

@petebacondarwin petebacondarwin force-pushed the linker-remove-i18n-options branch from 610b29d to eb14e11 Compare April 12, 2021 15:31
@zarend zarend added the area: compiler Issues related to `ngc`, Angular's template compiler label Apr 12, 2021
@ngbot ngbot bot added this to the Backlog milestone Apr 12, 2021
@petebacondarwin petebacondarwin removed the request for review from alxhub April 13, 2021 13:38
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 13, 2021
@zarend
Copy link
Contributor

zarend commented Apr 13, 2021

hi @petebacondarwin, I am unable to merge this because it has a merge conflict with the master branch 😢 .

There were three options being made available to users of the linker:

- ` enableI18nLegacyMessageIdFormat`
-  `i18nNormalizeLineEndingsInICUs`
- ` i18nUseExternalIds`

None of these should actually be configurable at linking time
because partially-linked libraries have tighter restrictions on
what i18n options can be used.

This commit removes those options from the `LinkerOptions` interface.
It was considered to add a check for backwards compatibilty to ensure
that if these options were being passed, and were different to the expected
defaults, we would throw an informative error. But from looking at the
Angular CLI (the only known client of the linker) it has never been setting
these options so they have already always been set to the defaults.

BREAKING CHANGE:

Linked libraries no longer generate legacy i18n message ids. Any downstream
application that provides translations for these messages, will need to
migrate their message ids using the `localize-migrate` command line tool.

Closes angular#40673
@petebacondarwin petebacondarwin force-pushed the linker-remove-i18n-options branch from eb14e11 to 1517e11 Compare April 13, 2021 20:08
@zarend zarend closed this in ba84fa6 Apr 13, 2021
@petebacondarwin petebacondarwin deleted the linker-remove-i18n-options branch April 13, 2021 21:13
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler breaking changes cla: yes compiler: linker refactoring Issue that involves refactoring or code-cleanup target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove i18n options from linker - hard error in partial compilation if not set to expected values.
4 participants