perf(common): remove unused methods from DomAdapter by crisbeto · Pull Request #41102 · angular/angular

crisbeto · 2021-03-06T09:49:13Z

The DomAdapter is present in all Angular apps and its methods aren't tree shakeable. These changes remove the methods that either aren't being used anymore or were only used by our own tests. Note that these changes aren't breaking, because the adapter is an internal API.

The following methods were removed:

getProperty - only used within our own tests.
log - Guaranteed to be defined on console.
logGroup and logGroupEnd - Only used in one place. It was placed in the DomAdapter for built-in null checking.
performanceNow - Only used in one place that has to be invoked through the browser console.
supportsCookies - Unused.
getCookie - Unused.
getLocation and getHistory - Only used in one place which appears to have access to the DOM already, because it had direct accesses to window. Furthermore, even if this was being used in a non-browser context already, the DominoAdapter was set up to throw an error.

The following APIs were changed to be more compact:

supportsDOMEvents - Changed to a readonly property.
remove - No longer returns the removed node.

itea-dev · 2021-03-06T10:28:39Z

There is a minor typo in the description.

Only used in one place that has to be invoked through the broser console.

crisbeto · 2021-03-06T10:53:54Z

packages/core/test/linker/security_integration_spec.ts

The DominoAdapter had a special case for property reads of href that turned them into getAttribute('href') so this check wasn't actually doing anything, as far as I could tell.

jelbourn

LGTM

Reviewed-for: size-tracking

jelbourn

LGTM

Reviewed-for: fw-security

packages/platform-browser/src/browser/browser_adapter.ts

packages/platform-server/src/domino_adapter.ts

AndrewKushnir · 2021-03-09T17:24:02Z

Presubmit.

AndrewKushnir · 2021-03-09T19:45:38Z

@crisbeto running tests in Google's codebase indicated that this change breaks a lot of targets (85%) due to the usage of removed methods in older versions of FlexLayout and deprecated @angular/http module. This would require some refactoring in Google's codebase (to avoid using removed methods) before landing the change.

crisbeto · 2021-03-09T20:32:13Z

getCookie has been restored with a comment about why it's there.

AndrewKushnir · 2021-03-10T01:31:07Z

@crisbeto FYI we've performed the necessary refactoring in Google's codebase to get it ready for this change. I will run more broad set of tests tonight to see if there are other places that'd require a cleanup and will keep you updated. Thank you.

AndrewKushnir · 2021-03-10T01:33:30Z

Btw, it looks like this PR also needs the fw-platform-server group approval, so may be @alxhub or @kyliau can have a look at these changes too when they get a chance? Thank you.

packages/platform-server/src/domino_adapter.ts

AndrewKushnir · 2021-03-10T03:29:46Z

Global Presubmit.

The `DomAdapter` is present in all Angular apps and its methods aren't tree shakeable. These changes remove the methods that either aren't being used anymore or were only used by our own tests. Note that these changes aren't breaking, because the adapter is an internal API. The following methods were removed: * `getProperty` - only used within our own tests. * `log` - Guaranteed to be defined on `console`. * `logGroup` and `logGroupEnd` - Only used in one place. It was in the DomAdapter for built-in null checking. * `logGroupEnd` - Only used in one place. It was placed in the DomAdapter for built in null checking. * `performanceNow` - Only used in one place that has to be invoked through the browser console. * `supportsCookies` - Unused. * `getCookie` - Unused. * `getLocation` and `getHistory` - Only used in one place which appears to have access to the DOM already, because it had direct accesses to `window`. Furthermore, even if this was being used in a non-browser context already, the `DominoAdapter` was set up to throw an error. The following APIs were changed to be more compact: * `supportsDOMEvents` - Changed to a readonly property. * `remove` - No longer returns the removed node.

AndrewKushnir · 2021-03-10T18:41:28Z

FYI, Global Presubmit is successful. I've started a new presubmit with a subset of targets to verify that the most recent changes are ok too and once presubmit is done, this PR should be ready to go.

AndrewKushnir · 2021-03-10T18:43:42Z

Note to Caretaker: PullApprove still wants the fw-security group review, even though Jeremy already approved this PR on behalf of that group in #41102 (review). This PR should be ready to go once final presubmits are completed.

AndrewKushnir · 2021-03-10T19:47:38Z

FYI, discussed with @crisbeto that this PR should go into master only to avoid any potential breakages in patch versions.

angular-automatic-lock-bot · 2021-04-10T16:07:46Z

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 Mar 6, 2021

crisbeto force-pushed the dom-adapter-cleanup branch 2 times, most recently from bfd3cc2 to c7b6bc4 Compare March 6, 2021 10:27

crisbeto force-pushed the dom-adapter-cleanup branch 2 times, most recently from 67e6231 to ca1fd92 Compare March 6, 2021 10:52

crisbeto commented Mar 6, 2021

View reviewed changes

crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package target: patch This PR is targeted for the next patch release labels Mar 6, 2021

ngbot bot added this to the Backlog milestone Mar 6, 2021

crisbeto marked this pull request as ready for review March 6, 2021 11:33

ngbot bot modified the milestone: Backlog Mar 6, 2021

pullapprove bot requested review from IgorMinar, alxhub, atscott, jelbourn and mhevery March 6, 2021 11:34

jelbourn approved these changes Mar 8, 2021

View reviewed changes

pullapprove bot requested a review from jelbourn March 8, 2021 19:38

jelbourn approved these changes Mar 8, 2021

View reviewed changes

atscott approved these changes Mar 8, 2021

View reviewed changes

alfaproject reviewed Mar 9, 2021

View reviewed changes

packages/platform-browser/src/browser/browser_adapter.ts Outdated Show resolved Hide resolved

alfaproject reviewed Mar 9, 2021

View reviewed changes

packages/platform-server/src/domino_adapter.ts Outdated Show resolved Hide resolved

crisbeto force-pushed the dom-adapter-cleanup branch 3 times, most recently from 80cfd1a to 259431d Compare March 9, 2021 16:50

crisbeto 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 Mar 9, 2021

crisbeto added the action: presubmit The PR is in need of a google3 presubmit label Mar 9, 2021

AndrewKushnir added the state: blocked label Mar 9, 2021

crisbeto force-pushed the dom-adapter-cleanup branch from 259431d to 9f3e671 Compare March 9, 2021 20:12

crisbeto removed the state: blocked label Mar 9, 2021

kyliau reviewed Mar 10, 2021

View reviewed changes

packages/platform-server/src/domino_adapter.ts Outdated Show resolved Hide resolved

crisbeto force-pushed the dom-adapter-cleanup branch from 9f3e671 to 6c12ac2 Compare March 10, 2021 07:21

kyliau approved these changes Mar 10, 2021

View reviewed changes

AndrewKushnir removed request for IgorMinar, alxhub and mhevery March 10, 2021 18:41

AndrewKushnir added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Mar 10, 2021

pullapprove bot requested review from IgorMinar and mhevery March 10, 2021 18:42

AndrewKushnir added risk: medium target: major This PR is targeted for the next major release and removed action: presubmit The PR is in need of a google3 presubmit target: patch This PR is targeted for the next patch release labels Mar 10, 2021

AndrewKushnir closed this in 3c66b10 Mar 10, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 10, 2021

Conversation

crisbeto commented Mar 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itea-dev commented Mar 6, 2021

Uh oh!

crisbeto Mar 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir commented Mar 9, 2021

Uh oh!

AndrewKushnir commented Mar 9, 2021

Uh oh!

crisbeto commented Mar 9, 2021

Uh oh!

AndrewKushnir commented Mar 10, 2021

Uh oh!

AndrewKushnir commented Mar 10, 2021

Uh oh!

Uh oh!

AndrewKushnir commented Mar 10, 2021

Uh oh!

AndrewKushnir commented Mar 10, 2021

Uh oh!

AndrewKushnir commented Mar 10, 2021

Uh oh!

AndrewKushnir commented Mar 10, 2021

Uh oh!

angular-automatic-lock-bot bot commented Apr 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

crisbeto commented Mar 6, 2021 •

edited

Loading

crisbeto Mar 6, 2021 •

edited

Loading