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

Minor UI package maintenance #609

Merged
merged 3 commits into from
Jun 20, 2019
Merged

Minor UI package maintenance #609

merged 3 commits into from
Jun 20, 2019

Conversation

msorens
Copy link
Contributor

@msorens msorens commented Jun 17, 2019

🔩 Description

Starting to think about an Angular upgrade (Angular 8 just came out and we are still on Angular 6).
Ran into various issues when attempting to upgrade that need more investigation, but this PR is at least a start to tidy up a couple minor things.

Upon upgrading tslint a few new linting errors appeared, and this PR resolves them.

  1. Error: Tried to lint file.js but found no valid, enabled rules for this file type and file path in the resolved configuration. The fix for that is to add a jsRules section to the tslint.json file, as described here: No valid rules have been specified palantir/tslint#3735

  2. The recommended content of the jsRule section from that suggested fix--to turn on the "no empty blocks" rule caused, a few instances of, yep (block is empty). It turns out that the content of that jsRules for the first fix is completely arbitrary; any rule will do, so I changed that to something innocuous (no-debugger), and that resolved this second issue.

  3. Error: no-use-before-declare is deprecated. This refers to a linting rule in tslint.json that is both expensive to compute and not at all needed in a ts application (would be in a plain js application). So just deleting that line from the config in tslint.json was the fix here. Reference: https://palantir.github.io/tslint/rules/no-use-before-declare/

👍 Definition of Done

  1. Sass builds complete without error.
  2. linting in automate-ui and in chef-ui-library returns no errors.

👟 Demo Script / Repro Steps

⛓️ Related Resources

✅ Checklist

  • Necessary tests added/updated?
  • Necessary docs added/updated?
  • Code actually executed?
  • Vetting performed (unit tests, lint, etc.)?

@msorens msorens requested a review from a team June 17, 2019 03:12
@msorens msorens requested a review from a team as a code owner June 17, 2019 03:12
@msorens msorens self-assigned this Jun 17, 2019
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the effort!

components/release-mgmt-dashboard/tslint.json Outdated Show resolved Hide resolved
@@ -10383,79 +10357,12 @@
"path-is-absolute": "1.0.1"
}
},
"har-schema": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I wonder what had dragged this in, and why it's gone now. 🤷‍♂

Copy link
Contributor

@tylercloke tylercloke left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

just blocking this so noone sees all the approvals and hits merge. we shouldn't mess with the release-management-dashboard in this way -- let's do it properly or not at all 😃

Signed-off-by: michael sorens <msorens@chef.io>
Signed-off-by: michael sorens <msorens@chef.io>
Signed-off-by: michael sorens <msorens@chef.io>
@msorens msorens requested a review from srenatus June 19, 2019 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants