Skip to content

Remove polyfills file #23938

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 26, 2022
Merged

Remove polyfills file #23938

merged 2 commits into from
Sep 26, 2022

Conversation

@alan-agius4
Copy link
Collaborator

@alan-agius4 alan-agius4 commented Sep 21, 2022

feat(@angular-devkit/build-angular): amend polyfills option in all builders to support an array of module specifiers

This is the ground work to be able to remove the polyfills.ts file which today is primarily used to add zone.js.

Usage examples:

polyfills: ['zone.js'],

polyfills: ['zone.js', 'zone.js/testing', 'src/polyfills.ts'],

feat(@schematics/angular): drop polyfills.ts file from new templates

With this change we drop the polyfills.ts from new application projects and add the polyfills directly in the angular.json. This is possible as now the polyfills option accept an array of module specifiers.

This change also fixes another open issue (#14432) which was caused by the missing polyfills file in the library test setup.

Closes #14432

@alan-agius4 alan-agius4 added the target: major This PR is targeted for the next major release label Sep 21, 2022
@alan-agius4 alan-agius4 force-pushed the polyfills-array branch 3 times, most recently from 70cdde2 to 4f20da4 Compare September 21, 2022 13:34
@e-oz
Copy link

e-oz commented Sep 22, 2022

Will it still be possible to define Zone.JS flags before importing Zone.JS?

@alan-agius4
Copy link
Collaborator Author

alan-agius4 commented Sep 22, 2022

Will it still be possible to define Zone.JS flags before importing Zone.JS?

You can still provide those in a polyfills.ts file.

@e-oz
Copy link

e-oz commented Sep 22, 2022

You can still provide those in a polyfills.ts file.

If I remember correctly, the order of imports matters here. Flags should be defined before zone.js import.

@alan-agius4
Copy link
Collaborator Author

alan-agius4 commented Sep 22, 2022 •
edited
Loading

You can order the files to imports as you'd needed.

Example:

polyfills: ['./zone-flags.ts', 'zone.js'],
polyfills: ['./zone-flags.ts', 'zone.js', './other-polyfills.ts', ...],

@alan-agius4 alan-agius4 force-pushed the polyfills-array branch 2 times, most recently from bf3813e to f31a70e Compare September 22, 2022 14:28
@e-oz
Copy link

e-oz commented Sep 22, 2022

Thanks a lot for answering :)

@alan-agius4 alan-agius4 requested a review from clydin September 22, 2022 15:24
@alan-agius4 alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 22, 2022
@alan-agius4 alan-agius4 marked this pull request as ready for review September 22, 2022 15:41
@alan-agius4 alan-agius4 force-pushed the polyfills-array branch 2 times, most recently from 202c566 to dfeaba8 Compare September 22, 2022 15:50
@alan-agius4 alan-agius4 added this to the v15 Feature Freeze milestone Sep 22, 2022
@alan-agius4 alan-agius4 force-pushed the polyfills-array branch 7 times, most recently from ad23fb8 to e634ff7 Compare September 22, 2022 17:46
clydin
clydin requested changes Sep 26, 2022
View reviewed changes
packages/angular_devkit/build_angular/src/builders/browser-esbuild/options.ts
const polyfillsEntryPoint = options.polyfills && path.join(workspaceRoot, options.polyfills);

// Currently esbuild do not support multiple files per entry-point
const [polyfillsEntryPoint, ...remainingPolyfills] = normalizePolyfills(
Copy link
Member

@clydin clydin Sep 26, 2022

Choose a reason for hiding this comment

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

Can the path resolving step be skipped here? The absolute paths will end up in the sourcemaps.
esbuild should be able to resolve the relative paths directly.

Copy link
Collaborator Author

@alan-agius4 alan-agius4 Sep 26, 2022

Choose a reason for hiding this comment

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

As per off-line convo, lets do this in a follow up

@angular-robot angular-robot bot added the feature Issue that requests a new feature label Sep 26, 2022
@alan-agius4
feat(@angular-devkit/build-angular): amend polyfills option in all …
bb1aace 
…builders to support an array of module specifiers

This is the ground work to be able to remove the `polyfills.ts` file which today is primarily used to add `zone.js`.

Usage examples:
```js
polyfills: ['zone.js'],

polyfills: ['zone.js', 'zone.js/testing', 'src/polyfills.ts'],
```
@alan-agius4 alan-agius4 requested a review from clydin September 26, 2022 15:46
@alan-agius4
feat(@schematics/angular): drop polyfills.ts file from new templates
1c80211 
With this change we drop the `polyfills.ts` from new application projects and add the polyfills directly in the `angular.json`. This is possible as now the `polyfills` option accept an array of module specifiers.

This change also fixes another open issue (angular#14432) which was caused by the missing polyfills file in the library test setup.

Closes angular#14432
@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 Sep 26, 2022
@ngbot
Copy link

ngbot bot commented Sep 26, 2022

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "ci/circleci: build-bazel-e2e" is failing
    pending status "ci/circleci: e2e-cli-npm" is pending
    pending status "ci/circleci: e2e-cli-win" is pending
If you want your PR to be merged, it has to pass all the CI checks.
If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@alan-agius4 alan-agius4 merged commit 597bfea into angular:main Sep 26, 2022
@alan-agius4 alan-agius4 deleted the polyfills-array branch September 26, 2022 16:27
@alan-agius4 alan-agius4 mentioned this pull request Sep 28, 2022
Closed
alan-agius4 added a commit to alan-agius4/angular that referenced this pull request Sep 30, 2022
@alan-agius4
fix(localize): add polyfill in polyfills array instead of polyfills.ts
f78ea82 
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.
AndrewKushnir pushed a commit to angular/angular that referenced this pull request Oct 3, 2022
@alan-agius4 @AndrewKushnir
fix(localize): add polyfill in polyfills array instead of polyfills.ts (
400a6b5 
#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 #47569
pterratpro pushed a commit to pterratpro/angular-fr that referenced this pull request Oct 4, 2022
@alan-agius4 @pterratpro
fix(localize): add polyfill in polyfills array instead of polyfills.ts (
0c388ce 
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
@alan-agius4 @pterratpro
fix(localize): add polyfill in polyfills array instead of polyfills.ts (
47db566 
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

angular-automatic-lock-bot bot commented Oct 27, 2022

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 Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@clydin clydin clydin approved these changes

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker feature Issue that requests a new feature target: major This PR is targeted for the next major release

Projects

None yet

Milestone

v15 Feature Freeze

Development

Successfully merging this pull request may close these issues.

Jasmine's clock mocks can't be used in library specs

3 participants

@alan-agius4 @e-oz @clydin