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

refactor(external-controller): Upgrade eslint config to "recommended" and fix violations in the code #21038

Merged

Conversation

Josmithr
Copy link
Contributor

The minimal-deprecated configuration has been long deprecated and is scheduled for deletion. This PR upgrades the package's configuration to "recommended" and fixes resulting violations.

@Josmithr Josmithr requested a review from a team May 10, 2024 00:21
@github-actions github-actions bot added area: build Build related issues area: examples Changes that focus on our examples base: main PRs targeted against main branch labels May 10, 2024
@github-actions github-actions bot removed the area: build Build related issues label May 10, 2024

// Incompatible with prettier
// TODO: remove this override once config dependency has been updated to a version with this rule disabled..
"unicorn/number-literal-case": "off",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Base config updated in #21037 to disable this rule.

@Josmithr Josmithr marked this pull request as ready for review May 10, 2024 00:22
Copy link
Contributor

@ChumpChief ChumpChief left a comment

Choose a reason for hiding this comment

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

Couple suggestions but looks good in general!

@@ -71,12 +71,18 @@ const containerSchema = {
function createDiceRollerControllerProps(map: ISharedMap): DiceRollerControllerProps {
return {
get: (key: string) => map.get(key) as number,
set: (key: string, value: any) => map.set(key, value),
on(event: "valueChanged", listener: (args: IValueChanged) => void) {
set: (key: string, value: unknown) => map.set(key, value),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question for my own understanding; why is unknown preferred over any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any allows users to call any properties or methods on the object, and the compiler assumes they all are valid and exist at runtime. Additionally, any is assignable to arguments of any type without validation. So, it promotes very unsafe code.

unknown doesn't allow you to assume anything about the object - no methods or properties may be called on an object of this type, and unknown is not assignable to any types except unknown and any. So, using unknown forces you to do validation before making any assumptions about what may or may not be there.

Copy link
Contributor

@WayneFerrao WayneFerrao left a comment

Choose a reason for hiding this comment

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

Just had a small question, but other than that, it looks good!

@Josmithr Josmithr merged commit a5f84ed into microsoft:main May 14, 2024
30 checks passed
@Josmithr Josmithr deleted the external-controller/upgrade-eslint-config branch May 14, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: examples Changes that focus on our examples base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants