fix(core): remove duplicated EMPTY_ARRAY constant by cexbrayat · Pull Request #40587 · angular/angular

cexbrayat · 2021-01-27T07:53:49Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

The codebase currently contains several EMPTY_ARRAY constants,
and they can end up in the bundle of an application.
A recent commit 6fbe219 tipped us off
as it introduced several noop occurrences in the golden symbol files.

What is the new behavior?

After investigating with @petebacondarwin,
we decided to remove the duplicated symbols.

This probably shaves only a few bytes,
but this commit removes the duplicated functions,
by always using the one in core/src/utils/empty.

Does this PR introduce a breaking change?

Yes
No

Other information

petebacondarwin

Smashing! Thanks @cexbrayat

atscott

Nice

gkalpak · 2021-01-27T15:21:01Z

packages/core/src/render3/empty.ts

Are we sure we don't mind that some EMPTY_ARRAYs will not be frozen in dev mode any more?

No worries: it now uses EMPTY_ARRAY from util/empty.ts which does exactly the same (see https://github.com/angular/angular/blob/a6971ba89adc253bfa4260036ee4a1e0bd76159f/packages/core/src/util/empty.ts)

Right. Then, are we sure we don't mind that some EMPTY_ARRAYs will now be frozen in dev mode? 😁

Ha! That's a tougher question to answer 😃. I think it should be fine but do you see an area where it might be problematic?

I would have thought that freezing in dev-mode is what we want. And possibly not bothering to freeze in prod mode.
The general idea being that we would catch errors that try to modify the object during testing in dev mode.

Agreed. OOC: is one of the test suites running with ngDevMode enabled?

My concern was more with operations that don't throw errors. For example, a = []; Object.freeze(a); a[0] = 'v'; will silently fail.

But looking at the code, there doesn't seem to be a place were EMPTY_ARRAY is modified. Ideally we should type it as ReadonlyArray, but this is out of scope for this PR 😃

Oh I always thought that a frozen object/array would throw an error in that case. TIL!

Actually in strict mode it will throw an error, apparently: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze#freezing_arrays

Ah, nice! That's more reassuring 😃

JoostK · 2021-01-27T17:15:18Z

This happens to address #28229 :-D

jessicajaniuk · 2021-01-27T18:43:13Z

Presubmit

@petebacondarwin

The codebase currently contains several `EMPTY_ARRAY` constants, and they can end up in the bundle of an application. A recent commit 6fbe219 tipped us off as it introduced several `noop` occurrences in the golden symbol files. After investigating with @petebacondarwin, we decided to remove the duplicated symbols. This probably shaves only a few bytes, but this commit removes the duplicated functions, by always using the one in `core/src/utils/empty`. PR Close #40587

@petebacondarwin

The codebase currently contains several `EMPTY_ARRAY` constants, and they can end up in the bundle of an application. A recent commit 6fbe219 tipped us off as it introduced several `noop` occurrences in the golden symbol files. After investigating with @petebacondarwin, we decided to remove the duplicated symbols. This probably shaves only a few bytes, but this commit removes the duplicated functions, by always using the one in `core/src/utils/empty`. PR Close #40587

mhevery · 2021-01-28T22:34:13Z

This change breaks //cloud/console/web/appengine/versions:karma_skip_snapshots_chrome-linux reverting

This reverts commit 34aa9c3.

This reverts commit cabe8be.

This reverts commit abdf25c.

mhevery · 2021-01-28T23:43:18Z

Do not merge this PR until http://b/178744068 is resolved. (Has been resolved 2/1/2021)

mhevery · 2021-02-01T18:48:40Z

presubmit

AndrewKushnir · 2021-02-25T05:13:09Z

@cexbrayat I've tried rebasing this PR using our tooling, but it detected that the original commit was already merged (and reverted later), so the rebase resulted in an empty branch :(

I can restore actual changes using previously merged commits, but I'd like to ask if you're ok re-creating this PR, so that the change is submitted under your name.

Note: I've also started a new presubmit and we'll use these results for the newly created PR.

Please let me know if you can help with re-creating this PR or you want me to handle this.

Thank you.

cexbrayat · 2021-02-25T10:16:52Z

@AndrewKushnir Sure, I opened #40991 let me know if that's OK.

Thanks for your help Andrew

angular-automatic-lock-bot · 2021-03-28T16:07:38Z

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.}

google-cla bot added the cla: yes label Jan 27, 2021

pullapprove bot requested a review from atscott January 27, 2021 07:53

cexbrayat force-pushed the fix/duplicate-empty-symbol branch 2 times, most recently from ae19b74 to 83dd9d2 Compare January 27, 2021 08:23

petebacondarwin approved these changes Jan 27, 2021

View reviewed changes

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release labels Jan 27, 2021

ngbot bot added this to the Backlog milestone Jan 27, 2021

atscott approved these changes Jan 27, 2021

View reviewed changes

gkalpak reviewed Jan 27, 2021

View reviewed changes

JoostK linked an issue Jan 27, 2021 that may be closed by this pull request

EMPTY_ARRAY used for identity checks, but multiple are declared in core #28229

Closed

jessicajaniuk added action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Jan 27, 2021

mhevery closed this in 34aa9c3 Jan 28, 2021

JoostK mentioned this pull request Jan 28, 2021

EMPTY_ARRAY used for identity checks, but multiple are declared in core #28229

Closed

mhevery reopened this Jan 28, 2021

mhevery added a commit that referenced this pull request Jan 28, 2021

Revert "fix(core): remove duplicated EMPTY_ARRAY constant (#40587)"

88f8ddd

This reverts commit 34aa9c3.

mhevery added a commit that referenced this pull request Jan 28, 2021

Revert "fix(core): remove duplicated EMPTY_ARRAY constant (#40587)"

552009e

This reverts commit cabe8be.

mhevery added a commit that referenced this pull request Jan 28, 2021

Revert "fix(core): remove duplicated EMPTY_ARRAY constant (#40587)"

e706f46

This reverts commit abdf25c.

mhevery added the state: blocked label Jan 28, 2021

mhevery removed the state: blocked label Feb 1, 2021

alan-agius4 added the action: presubmit The PR is in need of a google3 presubmit label Feb 22, 2021

AndrewKushnir closed this Feb 25, 2021

AndrewKushnir force-pushed the fix/duplicate-empty-symbol branch from 83dd9d2 to 6c05c80 Compare February 25, 2021 05:01

cexbrayat mentioned this pull request Feb 25, 2021

fix(core): remove duplicated EMPTY_ARRAY constant #40991

Closed

14 tasks

cexbrayat deleted the fix/duplicate-empty-symbol branch February 25, 2021 10:16

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 28, 2021

Conversation

cexbrayat commented Jan 27, 2021

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoostK commented Jan 27, 2021

Uh oh!

jessicajaniuk commented Jan 27, 2021

Uh oh!

mhevery commented Jan 28, 2021

Uh oh!

mhevery commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhevery commented Feb 1, 2021

Uh oh!

AndrewKushnir commented Feb 25, 2021

Uh oh!

cexbrayat commented Feb 25, 2021

Uh oh!

angular-automatic-lock-bot bot commented Mar 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mhevery commented Jan 28, 2021 •

edited

Loading