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

[EGRC-268] Establish Linting and Address Issues in OSCAL Viewer #34

Merged
merged 21 commits into from
Apr 9, 2021

Conversation

zclarkEDC
Copy link
Member

Overview

Set up ESLint to enforce the Airbnb React Style Guide.
Any linting errors will cause npm start and npm run build to fail.

Installation

npm install
npm run lint to see any errors.

Issues

There were over 400 problems that could not be solved with --fix linting command.
Many of these issues were resolved with rules in eslintrc.json:

 "rules": {
      "react/jsx-filename-extension": [1, { "extensions": [".js", ".jsx"] }],
      "react/prop-types": [0],
      "react/destructuring-assignment": [0],
      "no-unused-vars": "off",
      "no-shadow": "off",
      "react/jsx-props-no-spreading": "off"
      
    },

To see the issues in the console for yourself, just delete these rules and run npm run lint in the console.

Props issues

react/prop-types was especially troubling: any instance of props needed validation with PropTypes, and had to be destructured into individual variables.
It would require a massive amount of refactoring, basically every component would have to be rewritten and how props are passed between components would have to change.

For loops

The airbnb style guide is explicitly against using for loops, throwing a no-restricted-syntax at them.
There are instances of /* eslint-disable */

  • OSCALCatalog.js
  • OSCALControl .js
  • OSCALControlPart.js
  • OSCALMetadata.js

-Using the .eslintrc.json file, linted the entire project which
changes the formating of almost all files.
-Also had to add import React to most files using any .jsx
-Added rule allowing the use of .js or .jsx, this is a recommended
change based on many guides I have seen.
- Disabled prop types, and restructuring for React. Having these rules
in effect would require massive rewrite of the codebase. Basically it
would require that we remove any instance of "props.something" and
destructure it to a "something" object (using PropTypes). Disabling these
2 checks brought the error amount down from 315 to 58.
- Turned off multiline wrapping, for better readability
- Turned off one-expression-per line, improved readbility especially for
OSCALControlImplementationImplReq
Eslint has a hard time recognizing a variable from other React
components, as well as basic js varaibles like "event" and "error".
-App.test.js
-OSCALControl.js
-OSCALControlPart.js
-OSCALLoader.js
In OSCALControlPart
In OSCALMetadata
In OSCALSystemImplementation
This is not a permanent fix, need to investigate if we want to make a
rule for it.
@zclarkEDC zclarkEDC self-assigned this Apr 9, 2021
Copy link
Contributor

@rgauss rgauss left a comment

Choose a reason for hiding this comment

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

@zclarkEDC, can you create a Jira task along the lines of 'Re-Evaluate Linting Workarounds...'.

Some of the rules we're disabling seem like they should be addressed, but getting the consistency this PR addresses in place first is important.

@kkennedy26, if this goes in before the component PR(s) you may have to address any linting errors.

@zclarkEDC zclarkEDC merged commit ff46619 into develop Apr 9, 2021
@rgauss rgauss deleted the EGRC-268 branch May 12, 2021 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants