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

Enable no-unchecked-record-access rule for devtools-core #23718

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

RishhiB
Copy link
Contributor

@RishhiB RishhiB commented Jan 31, 2025

ESLint Configuration Changes:

  • Changed the rule for @fluid-internal/fluid/no-unchecked-record-access from "warn" to "error" in the .eslintrc.cjs file by removing the line

Needs Review:

  • Modified variable declarations in DataVisualization.ts:
    • Added explicit type annotation EditSharedObject | undefined for editorFunction variable to handle potential undefined values
    • Updated type safety for editor function lookup from attributes type
@fluidframework/devtools-core: error during command 'eslint src' (exit code 1)
@fluidframework/devtools-core: /workspaces/FluidFramework-1/packages/tools/devtools/devtools-core/src/data-visualization/DataVisualization.ts
@fluidframework/devtools-core:   266:27  error  Implicit typing derived from '.editors[undefined]' is not allowed. 'undefined' is an index signature type and 'undefined' may be undefined. Please provide an explicit type annotation including undefined or enable noUncheckedIndexedAccess  @fluid-internal/fluid/no-unchecked-record-access

@Copilot Copilot bot review requested due to automatic review settings January 31, 2025 17:34
@RishhiB RishhiB requested a review from a team as a code owner January 31, 2025 17:34
@RishhiB RishhiB self-assigned this Jan 31, 2025
@github-actions github-actions bot added the base: main PRs targeted against main branch label Jan 31, 2025

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

@RishhiB RishhiB requested review from jason-ha and a team January 31, 2025 17:34
Comment on lines 266 to 267
const editorFunction = this.editors[sharedObject.attributes.type];
const editorFunction: EditSharedObject | undefined =
this.editors[sharedObject.attributes.type];
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a valid pattern. See numerous prior conversations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didnt know whether to make this change, allow the PR to pass and write this in the Needs review section, or to not make this change and not allow CI to pass. I will remove these changes from my PRs

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably you should remove recognition of this pattern as acceptable to the lint rule - then maybe no one will try to use it and hide problems.

Copy link
Contributor Author

@RishhiB RishhiB Jan 31, 2025

Choose a reason for hiding this comment

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

I can do this, but if I remove recognition of it, users will keep using the pattern, right? Isnt that what we are trying to avoid?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what you are asking. We thought that we could use the pattern of T | undefined to tell TypeScript to treat the variable as possibly undefined. But that doesn't happen. TypeScript ignores the undefined because [without noUncheckedIndexAccess] it thinks the result is always defined. Code says T | undefined and TypeScript sees T only.
Similar for ? - basically ignored by TypeScript and therefore meaningless.
If someone codes foo: T | undefined = and both the linter rule and TypeScript ignore the undefined part, then the linter will point out the issue and the code will have to be fixed. That is the result we want.

@RishhiB RishhiB marked this pull request as draft February 6, 2025 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants