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

Make no unused properties checking maximally strict #12910

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Dec 4, 2024

Summary

  • Fixes oversight from previous cleanup and ensures we are still checking for unused data and computed properties
  • Replaces our own bespoke no-unused-methods linting rule with built in eslint-plugin-vue method option
  • Adds checks for deep data properties being unused
  • Adds checks for setup properties being unused
  • Fixes all incorrectly flagged @public methods
  • Adds @public to methods that are only referenced in beforeRouteEnter navigation hooks, as they are only referenced from the vm object
  • Cleans up all other unused properties flagged by the linter

Reviewer guidance

A lot of these seemed to fall into these four categories, which seemed fine to cleanup:

  • Copy pasting
  • Half finished thoughts that were never cleaned up
  • People exporting by rote everything defined in the setup method whether it was used or not outside the component
  • Things that were no longer used in the component and then never cleaned up from the setup function

However, it is also possible that some of these things were lost in the process of refactor. I will attempt to flag when I think this might be the case in self review.

@github-actions github-actions bot added DEV: renderers HTML5 apps, videos, exercises, etc. APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) DEV: frontend DEV: tools Internal tooling for development labels Dec 4, 2024
Copy link
Member Author

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Flagged some points for closer inspection.

@@ -122,13 +121,6 @@
ReportsControls,
},
mixins: [commonCoach, commonCoreStrings],
setup() {
const { saveTabsClick, wereTabsClickedRecently } = useCoachTabs();
Copy link
Member Author

Choose a reason for hiding this comment

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

Broader question - do we still need this composable at all? @AlexVelezLl

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm no, we can remove this.

return {
ariaChecked,
Copy link
Member Author

Choose a reason for hiding this comment

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

@nucleogenesis was this actually needed anywhere?

Copy link
Member

Choose a reason for hiding this comment

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

I think it probably should have been passed to KCheckbox to the :ariaChecked attr

Copy link
Member Author

Choose a reason for hiding this comment

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

This is handled internally by KCheckbox: https://github.com/learningequality/kolibri-design-system/blob/develop/lib/KCheckbox.vue#L15 after your PR: learningequality/kolibri-design-system#572 so this does seem safe to cleanup!

@@ -196,7 +196,6 @@
pageSizeNumber,
getLearningActivityIcon,
networkDevices,
availableSpace,
Copy link
Member Author

Choose a reason for hiding this comment

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

@AlexVelezLl @marcellamaki I am guessing this is OK as this gets displayed in MyDownloads below?

Copy link
Member

Choose a reason for hiding this comment

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

Yes @rtibbles :)

fetchResumableContentNodes,
fetchMoreResumableContentNodes,
} = useLearnerResources();
const { fetchResumableContentNodes } = useLearnerResources();
Copy link
Member Author

Choose a reason for hiding this comment

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

We could defer even this loading just to the child component.

Copy link
Member

Choose a reason for hiding this comment

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

meaning the card grid?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, useLearnerResources is used extensively inside the ResumableCardGrid, so it could just handle all of this!

@@ -194,19 +194,15 @@
availableLanguages,
availableLibraryCategories,
searchableLabels,
activeSearchTerms,
Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like this was all copy pasted from another component in this folder - @nucleogenesis should we move the common things into the useBaseSearch composable instead?

Copy link
Member

Choose a reason for hiding this comment

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

Yep definitely

@@ -214,8 +214,6 @@
searchableLabels,
activeSearchTerms,
windowIsLarge,
// This color is not in KDS but was specifically requested in the design
Copy link
Member Author

Choose a reason for hiding this comment

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

But then also never used here! cc @nucleogenesis

Copy link
Member

Choose a reason for hiding this comment

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

Missed this during clean-up - thankful for the new maximally strict checker :)

@@ -226,7 +226,7 @@ module.exports = {
'vue/no-lone-template': OFF,
'vue/match-component-file-name': ERROR,
'vue/component-options-name-casing': [ERROR, 'PascalCase'],
'vue/no-unused-properties': ERROR,
'vue/no-unused-properties': [ERROR, { groups: ['props', 'data', 'computed', 'methods', 'setup'], deepData: true, ignorePublicMembers: true }],
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the linting rule specification.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

I don't have any major concerns about these changes, other than, "Yikes i'm glad we're doing this, there's a lot of code that's not used that's potentially making stuff way more confusing!"

Other than that, I've noted to places to myself to investigate about potential regressions (that the unused code might indicate), and will follow up and comment back, and link issues if I open any

);
return displaySectionTitle(activeSection.value, activeSectionIndex);
});

Copy link
Member

Choose a reason for hiding this comment

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

@nucleogenesis there's a fair amount of stuff here that's unused in the create exam page, and I think (?) you might be best positioned to take a look and confirm what of this was just leftover from refactoring and other adjustments at the end of 0.17. It seems mostly like that, but... could you just take an extra look through this? I think you will have a better sense than I do.

const activityKeys = Object.keys(activityRefs);
const firstActivity = activityKeys.find(key => key.endsWith('0'));
if (firstActivity) {
activityKeys[firstActivity].value.focusFirstEl();
Copy link
Member

Choose a reason for hiding this comment

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

This seems like something we may have updated to be better managed, so therefore not needed and just not cleaned up, but it also makes me wonder if we lost some accessibility stuff here due to a regression. I will see if I can trace back a bit and remember in the git history

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, although I suspect this might have been copy pasta from elsewhere - as the reference to sidepanel content (when this does not use a side panel at all) and activityRefs made me suspect that this is coming from something not at all related to showing a grid of resumable content.

Copy link
Member

Choose a reason for hiding this comment

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

yes, that's a good point

fetchResumableContentNodes,
fetchMoreResumableContentNodes,
} = useLearnerResources();
const { fetchResumableContentNodes } = useLearnerResources();
Copy link
Member

Choose a reason for hiding this comment

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

meaning the card grid?

@rtibbles
Copy link
Member Author

Rebased and force pushed.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

ty Richard

@rtibbles rtibbles merged commit deecb99 into learningequality:develop Dec 13, 2024
37 of 39 checks passed
@rtibbles rtibbles deleted the no_unused_all branch December 16, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: frontend DEV: renderers HTML5 apps, videos, exercises, etc. DEV: tools Internal tooling for development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants