Skip to content
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

fix(localize): add polyfill in polyfills array instead of polyfills.ts #47569

Closed
wants to merge 1 commit into from

Conversation

alan-agius4
Copy link
Contributor

With the recent changes in the Angular CLI (angular/angular-cli#23938) the polyfills option accepts module path that are resolved using Node module resolution. Also, the polyfills.ts file is no longer generated by default.

This commit changes the way on how the polyfill is added to the projects.

@alan-agius4 alan-agius4 added the target: major This PR is targeted for the next major release label Sep 28, 2022
@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Sep 28, 2022

Blocked on #47584

@ngbot ngbot bot added this to the Backlog milestone Sep 29, 2022
alan-agius4 added a commit to alan-agius4/angular that referenced this pull request Sep 29, 2022
This update is needed to implement the changes in `ng add localize` angular#47569
alan-agius4 added a commit to alan-agius4/angular that referenced this pull request Sep 29, 2022
This update is needed to implement the changes in `ng add localize` angular#47569
alan-agius4 added a commit to alan-agius4/angular that referenced this pull request Sep 29, 2022
- This update is needed to implement the changes in `ng add localize` angular#47569
- Add missing `root` options to all `angular.json`, this is required as otherwise the angular.json validation will fail.
- Remove `require.context` from test.ts integration test, as this is no longer needed.
- Update payloads golden files.
alan-agius4 added a commit to alan-agius4/angular that referenced this pull request Sep 29, 2022
- This update is needed to implement the changes in `ng add localize` angular#47569
- Add missing `root` options to all `angular.json`, this is required as otherwise the angular.json validation will fail.
- Remove `require.context` from test.ts integration test, as this is no longer needed.
- Update payloads golden files.
alan-agius4 added a commit to alan-agius4/angular that referenced this pull request Sep 29, 2022
- This update is needed to implement the changes in `ng add localize` angular#47569
- Add missing `root` options to all `angular.json`, this is required as otherwise the angular.json validation will fail.
- Remove `require.context` from test.ts integration test, as this is no longer needed.
- Update payloads golden files.
alan-agius4 added a commit to alan-agius4/angular that referenced this pull request Sep 30, 2022
- This update is needed to implement the changes in `ng add localize` angular#47569
- Add missing `root` options to all `angular.json`, this is required as otherwise the angular.json validation will fail.
- Remove `require.context` from test.ts integration test, as this is no longer needed.
- Update payloads golden files.
AndrewKushnir pushed a commit that referenced this pull request Sep 30, 2022
- This update is needed to implement the changes in `ng add localize` #47569
- Add missing `root` options to all `angular.json`, this is required as otherwise the angular.json validation will fail.
- Remove `require.context` from test.ts integration test, as this is no longer needed.
- Update payloads golden files.

PR Close #47584
With the recent changes in the Angular CLI (angular/angular-cli#23938) the polyfills option accepts module path that are resolved using Node module resolution. Also, the polyfills.ts file is no longer generated by default.

This commit changes the way on how the polyfill is added to the projects.
@alan-agius4 alan-agius4 marked this pull request as ready for review September 30, 2022 18:11
@pullapprove pullapprove bot requested a review from dgp1130 September 30, 2022 18:11
@alan-agius4 alan-agius4 requested review from AndrewKushnir and removed request for dgp1130 September 30, 2022 18:12
@alan-agius4 alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 30, 2022
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@alan-agius4 the changes look good 👍

Quick question: do we have tests to cover error cases (file that we try to patch is not found, the value of the polyfills field is unexpected, etc)? Just curious what the DX would look like in those cases and if there is a need to improve error messages, etc (in separate PR(s)).

@alan-agius4
Copy link
Contributor Author

uick question: do we have tests to cover error cases (file that we try to patch is not found, the value of the polyfills field is unexpected, etc)? Just curious what the DX would look like in those cases and if there is a need to improve error messages, etc (in separate PR(s)).

In general even during migrations we expect the project/builder target to be in a working state. This is because this is validated when running the CLI and an error would be displayed when there are invalid options.

In the case a file is not found an error will be displayed.

@alan-agius4 alan-agius4 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 Oct 1, 2022
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit 400a6b5.

@alan-agius4 alan-agius4 deleted the localize-schematic branch October 3, 2022 15:53
pterratpro pushed a commit to pterratpro/angular-fr that referenced this pull request Oct 4, 2022
)

- This update is needed to implement the changes in `ng add localize` angular#47569
- Add missing `root` options to all `angular.json`, this is required as otherwise the angular.json validation will fail.
- Remove `require.context` from test.ts integration test, as this is no longer needed.
- Update payloads golden files.

PR Close angular#47584
pterratpro pushed a commit to pterratpro/angular-fr that referenced this pull request Oct 4, 2022
angular#47569)

With the recent changes in the Angular CLI (angular/angular-cli#23938) the polyfills option accepts module path that are resolved using Node module resolution. Also, the polyfills.ts file is no longer generated by default.

This commit changes the way on how the polyfill is added to the projects.

PR Close angular#47569
pterratpro pushed a commit to pterratpro/angular-fr that referenced this pull request Oct 4, 2022
)

- This update is needed to implement the changes in `ng add localize` angular#47569
- Add missing `root` options to all `angular.json`, this is required as otherwise the angular.json validation will fail.
- Remove `require.context` from test.ts integration test, as this is no longer needed.
- Update payloads golden files.

PR Close angular#47584
pterratpro pushed a commit to pterratpro/angular-fr that referenced this pull request Oct 4, 2022
angular#47569)

With the recent changes in the Angular CLI (angular/angular-cli#23938) the polyfills option accepts module path that are resolved using Node module resolution. Also, the polyfills.ts file is no longer generated by default.

This commit changes the way on how the polyfill is added to the projects.

PR Close angular#47569
@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 Nov 3, 2022
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: i18n target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants