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

Deprecate legacy functions with modern alternatives and prune dead code #2418

Closed
aaemnnosttv opened this issue Nov 24, 2020 · 8 comments
Closed
Labels
P1 Medium priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature Type: Infrastructure Engineering infrastructure & tooling
Milestone

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Nov 24, 2020

Feature Description

There are a number of legacy functions in the codebase which have newer implementations which should be used instead for all new usage.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • All legacy functions which are no longer used should be deleted
    • Imports/exports and tests do not count as usage. In this case, only references in source files/components should be considered
    • Please list these instances in the IB
  • Any remaining legacy functions with a modern alternative (e.g. getSiteKitAdminURL - replaced by select( CORE_SITE ).getAdminURL(...)) should be annotated as @deprecated n.e.x.t Use x instead.
    • Please list these instances in the IB
    • Most (if not all) of these will likely be under assets/js/util/index.js

Implementation Brief

Delete the following from assets/js/util:

  • refresh-authentication.js
  • average-object-list-value.js
  • cache-data.js

Using assets/js/util/index.js delete the following functions:

  • removeURLFallBack
  • removeURLParameter
  • getDaysBetweenDates
  • validateOptimizeID

Delete the following from assets/js/util/test:

  • removeURLParameter.js
  • getDaysBetweenDates.js
  • validateOptimizeID.js
  • getSiteKitAdminURL.js
  • averageObjectListValue.js

In the following components remove the import of getSiteKitAdminURL and replace calls to it with the store equivalent useSelect( ( select ) => select( CORE_SITE ).getAdminURL( page , args ) ). In each case add imports for useSelect and CORE_SITE:

  • js/components/data-table
  • js/components/legacy-setup/SetupUsingGCP
  • js/components/setup/ModuleSetup
  • js/modules/adsense/components/dashboard/LegacyAdsenseDashboardMainSummary

In js/util/index remove the getSiteKitAdminURL function and its tests in assets/js/util/test/getSiteKitAdminURL.js

Test Coverage

  • run js tests and check that they pass.

Visual Regression Changes

  • n/a

QA Brief

As per the IB, check:

  • util files have been deleted
  • functions have been removed for index.js
  • associated tests have been deleted
  • calls to getSiteKitAdminURL have been replaced

Also check js and VRT tests pass.

Changelog entry

  • N/A
@aaemnnosttv aaemnnosttv added P2 Low priority Type: Enhancement Improvement of an existing feature labels Nov 24, 2020
@felixarntz
Copy link
Member

@aaemnnosttv Can you give some examples of which functions you're thinking about?

@aaemnnosttv
Copy link
Collaborator Author

@felixarntz off the top of my head: refreshAuthentication, getReAuthURL, getSiteKitAdminURL, and probably a few others in the top-level utils.

@aaemnnosttv
Copy link
Collaborator Author

We actually have a number of utils which are completely unused now, such as toggleConfirmModuleSettings and removeURLParameter too which would be good to clean up. I'll work on compiling a list for the ACs.

@felixarntz felixarntz added the Type: Infrastructure Engineering infrastructure & tooling label Mar 22, 2021
@aaemnnosttv aaemnnosttv changed the title Deprecate legacy functions with modern alternatives Deprecate legacy functions with modern alternatives and prune dead code Apr 1, 2021
@Hazlitte Hazlitte assigned Hazlitte and unassigned Hazlitte Jun 22, 2021
@felixarntz felixarntz added P1 Medium priority and removed P2 Low priority labels Jun 29, 2021
@fhollis fhollis modified the milestone: Sprint 53 Jul 2, 2021
@tofumatt tofumatt self-assigned this Jul 7, 2021
@eclarke1 eclarke1 added this to the Sprint 53 milestone Jul 7, 2021
@tofumatt
Copy link
Collaborator

tofumatt commented Jul 7, 2021

IB ✅

I wonder about "annotate the getSiteKitAdminURL function with @deprecated n.e.x.t Use selector getAdminURL instead." in the ACs though. The function won't be used anywhere in the codebase anymore, so why not remove it entirely?

Handing this over to @aaemnnosttv as he touched the ACs most recently.

@aaemnnosttv Any reason to keep that function around instead of removing it and its test file (assets/js/util/test/getSiteKitAdminURL.js)? It isn't used anyplace else anymore:

Screenshot 2021-07-07 at 14 48 18

@tofumatt tofumatt assigned aaemnnosttv and tofumatt and unassigned tofumatt and aaemnnosttv Jul 7, 2021
@tofumatt
Copy link
Collaborator

tofumatt commented Jul 7, 2021

(The ACs mentioned to deprecate anything that was still being used, but in this case getSiteKitAdminURL is not, so I've amended the IB because it won't be used anywhere.)

IB ✅

@tofumatt tofumatt removed their assignment Jul 7, 2021
@Hazlitte Hazlitte self-assigned this Jul 7, 2021
@Hazlitte Hazlitte added the QA: Eng Requires specialized QA by an engineer label Jul 8, 2021
@Hazlitte
Copy link
Contributor

Hazlitte commented Jul 8, 2021

@tofumatt During execution I have discovered that SetupUsingGCP is a class component so the call getSiteKitAdminURL cannot be replaced with the useSelect hook. Do you think it would be best to therefor leave the geSiteKitAdminURL function and tests for now and remove them when this component is refactored?

@tofumatt
Copy link
Collaborator

tofumatt commented Jul 8, 2021

In that case let's use the withSelect Higher-Order Component to get the data from the data store. It behaves similarly to mapStateToProps from Redux, and will let us get data from the data store and remove the legacy function.

Better to rely on that everywhere and remove the legacy component than to keep a lot of stuff around for that one file.

You can see an example of withSelect usage in our codebase here:

return withSelect( ( select ) => {
return {
dateRange: select( CORE_USER ).getDateRange(),
dateRangeLength: select( CORE_USER ).getDateRangeNumberOfDays(),
};
} )( NewComponent );
};

@Hazlitte Hazlitte removed their assignment Jul 12, 2021
@tofumatt tofumatt assigned tofumatt and Hazlitte and unassigned tofumatt Jul 12, 2021
@Hazlitte Hazlitte assigned tofumatt and unassigned Hazlitte Jul 13, 2021
@tofumatt tofumatt removed their assignment Jul 14, 2021
@danielgent danielgent self-assigned this Jul 15, 2021
@danielgent
Copy link
Contributor

QA ✅

Confirming all code removed as per IB

Tests and VRTs all pass on develop

@danielgent danielgent removed their assignment Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

7 participants