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

Clean up JS #788

Open
rob-3 opened this issue Feb 23, 2022 · 1 comment
Open

Clean up JS #788

rob-3 opened this issue Feb 23, 2022 · 1 comment
Assignees
Labels
cleanup javascript Pull requests that update Javascript code

Comments

@rob-3
Copy link
Contributor

rob-3 commented Feb 23, 2022

We should take a look through the JavaScript side of the project and clean up things like unused variables and fix any unjustified warnings.

We could also consider updating to function components, but that may be more of a long-term goal. The class components we have are still 100% supported by React.

@rob-3 rob-3 added cleanup javascript Pull requests that update Javascript code labels Feb 23, 2022
@rob-3
Copy link
Contributor Author

rob-3 commented Apr 19, 2022

We may want to consider splitting these into separate issues, as the list is pretty long.

This involves cleaning up some of the code in assets/js/. The main items are:

  • Rewriting the classes in assets/js/Services into plain functions. There is no good reason for these classes with no fields or state to exist in JavaScript, which has free functions. This is basically turning things like:
export default class Ufixit {
  returnIssueForm(activeIssue) {
    if (activeIssue) {
      if (UfixitForms.hasOwnProperty(activeIssue.scanRuleId)) {
        if (!activeIssue.sourceHtml.includes("data-type-module")) {
          return UfixitForms[activeIssue.scanRuleId]
        }
      }
    }

    return UfixitReviewOnly
  }
}

into

export function returnIssueForm(activeIssue) {
  if (activeIssue) {
    if (UfixitForms.hasOwnProperty(activeIssue.scanRuleId)) {
      if (!activeIssue.sourceHtml.includes("data-type-module")) {
        return UfixitForms[activeIssue.scanRuleId]
      }
    }
  }

  return UfixitReviewOnly
}

and updating the usages and imports elsewhere. You will also get to remove a bunch of instances of this after this is done.

  • Removing any unused imports
  • Removing any unused variables/parameters
  • Remove use of eval()! Using eval() is a bad idea is virtually all circumstances
  • Modernize JS style (remove var, use arrow functions where appropriate, etc)
  • Consider using public fields syntax for class components to avoid the need for .bind() everywhere
  • Update JS dependencies. Several packages in package.json are not the latest version or even the latest minor version. yarn upgrade can do some changes, but things like the @instructure/ui ilbraries need to be manually bumped and tested and any breaking changes fixed.
  • (Possibly) Updating to function components. Someone with working knowledge of class-based and function-based React components will need to do this
  • (Possibly) Implement some kind of JavaScript style guide for the repo and consider using a code formatter like Prettier to enforce that style. Currently, files have things like mixed 2 and 4 spaces and semicolons strewn at random.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup javascript Pull requests that update Javascript code
Projects
None yet
Development

No branches or pull requests

2 participants