Drop unused code #5067

fregante · 2023-01-22T10:22:15Z

What this PR does

This PR deletes unused code + drops exports that aren't imported anywhere (which helps with the automated detection of first part)

Discussion

I found a lot of surprisingly-unused code in the extension, some dating back to the original commit, replaced by modern alternatives or simply no longer useful. Some of this code requires maintenance via linting and missed "TODOs", so keeping it is counterproductive.

The tool also found more recent code (~6-7 months ago) which I did not delete.

Reviewer notes

To keep reviews speedy, if you see anything potentially useful again in the future, leave a comment with just "keep" and I'll restore with any of its dependencies. 👍

It's also possible that this PR highlights some issues, so please leave a comment if something doesn't look right.

One of the surprises is that we had two identical SelectorSelectorField components and neither one of them was used 🤔

❯ ts-prune > a ❯ cat a | rg -e '([^:]+).+ - (\w+) $used in module$' --only-matching --no-filename --replace 'sed -i "" "s/export type $2/type $2/" $1' | sh also with const and function instead of type ❯ git checkout -- src/**/*.stories.*

codecov · 2023-01-22T10:22:26Z

Codecov Report

Merging #5067 (485766a) into main (802dd08) will increase coverage by 0.60%.
The diff coverage is 68.85%.

@@            Coverage Diff             @@
##             main    #5067      +/-   ##
==========================================
+ Coverage   59.70%   60.30%   +0.60%     
==========================================
  Files         972      961      -11     
  Lines       29404    29064     -340     
  Branches     5636     5557      -79     
==========================================
- Hits        17555    17527      -28     
+ Misses      11849    11537     -312

Impacted Files	Coverage Δ
src/analysis/analysisVisitors/renderersAnalysis.ts	`100.00% <ø> (+3.22%)`	⬆️
...rc/analysis/analysisVisitors/varAnalysis/varMap.ts	`97.87% <ø> (+2.03%)`	⬆️
src/auth/authUtils.ts	`45.83% <ø> (ø)`
src/auth/useRequiredPartnerAuth.ts	`90.00% <ø> (ø)`
src/background/contextMenus.ts	`56.57% <ø> (ø)`
src/background/dataStore.ts	`0.00% <0.00%> (ø)`
src/background/externalProtocol.ts	`0.00% <0.00%> (ø)`
src/background/partnerIntegrations.ts	`53.84% <ø> (ø)`
src/blocks/PipelineExpressionVisitor.ts	`100.00% <ø> (ø)`
src/blocks/PipelineVisitor.ts	`100.00% <ø> (ø)`
... and 90 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

fregante · 2023-01-22T10:23:40Z

src/analysis/analysisVisitors/renderersAnalysis.ts

@@ -26,7 +26,7 @@ import { PipelineFlavor } from "@/pageEditor/pageEditorTypes";

 export const MULTIPLE_RENDERERS_ERROR_MESSAGE =
  "A panel can only have one renderer. There are one or more other renderers configured for this extension.";
-export const RENDERER_MUST_BE_LAST_BLOCK_ERROR_MESSAGE =
+const RENDERER_MUST_BE_LAST_BLOCK_ERROR_MESSAGE =


"This export could be useful!"

If/when it is, re-exporting it takes seconds

fregante · 2023-01-22T10:28:39Z

src/components/addBlockModal/QuickAdd.tsx

@@ -1,61 +0,0 @@
-/*


This module for example has a copy in /brickModalNoTags/

fregante · 2023-01-22T10:29:47Z

src/components/fields/schemaFields/BooleanField.tsx

-import SwitchButtonWidget from "@/components/form/widgets/switchButton/SwitchButtonWidget";
-import { makeLabelForSchemaField } from "@/components/fields/schemaFields/schemaFieldUtils";
-
-const BooleanField: React.FunctionComponent<SchemaFieldProps> = (props) => {


I think the last usage was dropped in #2915

fregante · 2023-01-22T10:30:26Z

src/components/fields/schemaFields/SelectorSelectorField.tsx

@@ -1,38 +0,0 @@
-/*


Last usage dropped in #4071

fregante · 2023-01-22T10:33:47Z

src/contentScript/pageEditor.ts

@@ -239,86 +212,6 @@ export async function runRendererBlock(
  }
 }

-export async function runReaderBlock({


This is an example of function that probably hasn't been used since 2021 but received several rounds maintenance

fregante · 2023-01-22T10:35:29Z

src/vendors/globalSearch.ts

@@ -1,53 +0,0 @@
-export function globalSearch(


A whole vendor dependency could now be dropped!

twschiller · 2023-01-22T16:31:56Z

The tool also found more recent code

Which tool did you run? Is it eslint? Would be nice to mark which functions must be exported b/c they're used by the app. And then enforce no unnecessary exports/dead code in CI

twschiller · 2023-01-22T16:33:23Z

src/contentScript/contentScriptCore.ts

@@ -54,7 +52,6 @@ export async function init(): Promise<void> {
  registerBuiltinBlocks();
  registerContribBlocks();

-  addListenerForUpdateSelectedElement();


@fregante Why is this removed?

If you follow the imported file you'll reach one of the functions that was unused. First I deleted that one, then the file became unused, then this import did nothing. It's related to one of the pageScript WITH_WINDOW functions or something like that.

The tool is ts-prune

It can't be automated because it has a lot of false positives and it doesn't actually edit the files. I just clicked its list and looked at the inline blame on my IDE

It can't be automated because it has a lot of false positives

I ran the report, and it doesn't look like many false positives. There's way to ignore file patterns: --ignore '.test.tsx?|.stories.tsx?' and also ignore individual lines (which would be useful for API surface area called by extension)

I'll ping @mnholtz and @alanb43 as a potential tool to add to our CI

twschiller · 2023-01-22T16:33:51Z

src/contentScript/messenger/api.ts

 export const detectFrameworks = getMethod("DETECT_FRAMEWORKS");

 export const runBlock = getMethod("RUN_SINGLE_BLOCK");
 export const runRendererBlock = getMethod("RUN_RENDERER_BLOCK");
-export const runReaderBlock = getMethod("RUN_READER_BLOCK");


twschiller · 2023-01-22T16:37:13Z

src/runtime/renderers.ts

@@ -40,7 +40,7 @@ export type RendererOptions = {
 * Returns true if `literalOrTemplate` includes any template expressions that would be replaced by `context`.
 * @param literalOrTemplate the string literal or Nunjucks/Handlebars template.
 */
-export function containsTemplateExpression(literalOrTemplate: string): boolean {
+function containsTemplateExpression(literalOrTemplate: string): boolean {


This logic is is replicated in 3-4 places (search for .includes("{{")). We might want to consolidate

Feel free to open an issue or PR, I'm not familiar with this code. It's best to leave logic changes out of large "cleanup" PRs so they're easier to follow

twschiller · 2023-01-22T16:37:41Z

src/services/api.ts

@@ -437,7 +436,6 @@ export const {
  useUpdatePackageMutation,
  useDeletePackageMutation,
  useListPackageVersionsQuery,
-  useUpdateScopeMutation,


This is likely used by the app? Or is it repeated in the app?

That I don't know unfortunately. I can open a temporary App PR based on this branch to find out

It seems that this export is used locally but due to the way it's being exported it's not being detected correctly by ts-prune.

useGetGroupsQuery is used by the app but it seems to be repeated there.

I reverted the changes made to this file anyway.

twschiller · 2023-01-22T16:40:20Z

@fregante looks good overall. Which tool are you using for the analysis?

Please check to make sure the app compiles with this branch before merging. My hunch is it wont
See comment here: https://github.com/pixiebrix/pixiebrix-extension/pull/5067/files#r1083495749

fregante · 2023-01-23T01:49:29Z

All good, you can see the full test suite on https://github.com/pixiebrix/pixiebrix-app/actions/runs/3982507927/jobs/6827031840

There was only one missed export:

pixiebrix-app/src/auth/authUtils.ts:19:10 - error TS2459: Module '"@/auth/authUtils"' declares 'selectOrganizations' locally, but it is not exported.

 import { selectOrganizations } from "@/auth/authUtils";

        ~~~~~~~~~~~~~~~~~~~


pixiebrix-extension/src/auth/authUtils.ts:22:10

 function selectOrganizations(

            ~~~~~~~~~~~~~~~~~~~

'selectOrganizations' is declared here.

github-actions · 2023-01-23T13:15:00Z

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

twschiller · 2023-01-23T13:15:39Z

src/auth/authUtils.ts

@@ -19,6 +19,7 @@ import { type Me } from "@/types/contract";
 import { type UserDataUpdate, type AuthState } from "@/auth/authTypes";
 import { type UUID } from "@/core";

+// Used by the app


@fregante it's better to use the ts-prune-ignore-next with a comment so that it doesn't show up in future reports

https://github.com/nadeesha/ts-prune#how-do-i-ignore-a-specific-identifier

fregante added 4 commits January 22, 2023 17:26

Don't export unless needed

75d1ed3

❯ ts-prune > a ❯ cat a | rg -e '([^:]+).+ - (\w+) $used in module$' --only-matching --no-filename --replace 'sed -i "" "s/export type $2/type $2/" $1' | sh also with const and function instead of type ❯ git checkout -- src/**/*.stories.*

Pass 1 + 2

0a5f81e

Lint

530de13

fregante added developer experience low priority labels Jan 22, 2023

fregante requested a review from twschiller January 22, 2023 10:22

fregante assigned fregante and twschiller Jan 22, 2023

fregante changed the title ~~Don't export unless needed~~ Drop unused code Jan 22, 2023

fregante commented Jan 22, 2023

View reviewed changes

Restore changes in vendors

515e290

vercel bot deployed to Preview January 22, 2023 13:57 View deployment

twschiller reviewed Jan 22, 2023

View reviewed changes

Restore selectOrganizations export

1eeb568

vercel bot deployed to Preview January 23, 2023 01:40 View deployment

Restore services/api.ts

485766a

vercel bot deployed to Preview January 23, 2023 01:56 View deployment

twschiller self-requested a review January 23, 2023 13:14

twschiller approved these changes Jan 23, 2023

View reviewed changes

twschiller reviewed Jan 23, 2023

View reviewed changes

twschiller added this to the 1.7.18 milestone Jan 23, 2023

twschiller merged commit e8321bb into main Jan 24, 2023

twschiller deleted the F/lint/don-t-export-unused branch January 24, 2023 13:53

This was referenced Dec 22, 2023

#7175: fix variable popover default setting #7176

Merged

Add dead code/unused export checks to CI #7177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop unused code #5067

Drop unused code #5067

fregante commented Jan 22, 2023

codecov bot commented Jan 22, 2023 •

edited

Loading

fregante Jan 22, 2023

fregante Jan 22, 2023 •

edited

Loading

fregante Jan 22, 2023

fregante Jan 22, 2023

fregante Jan 22, 2023

fregante Jan 22, 2023

twschiller commented Jan 22, 2023 •

edited

Loading

twschiller Jan 22, 2023

fregante Jan 22, 2023 •

edited

Loading

twschiller Jan 22, 2023

twschiller Jan 22, 2023

twschiller Jan 22, 2023

fregante Jan 22, 2023

twschiller Jan 22, 2023

fregante Jan 22, 2023

fregante Jan 23, 2023 •

edited

Loading

twschiller commented Jan 22, 2023 •

edited by fregante

Loading

fregante commented Jan 23, 2023

github-actions bot commented Jan 23, 2023

twschiller Jan 23, 2023

Drop unused code #5067

Drop unused code #5067

Conversation

fregante commented Jan 22, 2023

What this PR does

Discussion

Reviewer notes

codecov bot commented Jan 22, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

fregante Jan 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller commented Jan 22, 2023 • edited Loading

Choose a reason for hiding this comment

fregante Jan 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

twschiller commented Jan 22, 2023 • edited by fregante Loading

fregante commented Jan 23, 2023

github-actions bot commented Jan 23, 2023

Choose a reason for hiding this comment

codecov bot commented Jan 22, 2023 •

edited

Loading

fregante Jan 22, 2023 •

edited

Loading

twschiller commented Jan 22, 2023 •

edited

Loading

fregante Jan 22, 2023 •

edited

Loading

fregante Jan 23, 2023 •

edited

Loading

twschiller commented Jan 22, 2023 •

edited by fregante

Loading