Skip to content

WORLDSERVICE-225: Adds linting rules and standards around optional chaining #12646

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

Merged
merged 14 commits into from
Apr 16, 2025

Conversation

HarveyPeachey
Copy link
Contributor

@HarveyPeachey HarveyPeachey commented Apr 15, 2025

Resolves WORLDSERVICE-225

Summary

Adds linting rules and coding standards for Optional Chaining.

Code changes

  • Adds no-unsafe-optional-chaining lint rule
  • Adds @typescript-eslint/prefer-optional-chain rule
  • Removes .storybook folder from being linted temporarily
  • Adds new coding standard for optional chaining

Developer Checklist

  • UX
    • UX Criteria met (visual UX & screenreader UX)
  • Accessibility
    • Accessibility Acceptance Criteria met
    • Accessibility swarm completed
    • Component Health updated
    • P1 accessibility bugs resolved
    • P2/P3 accessibility bugs planned (if not resolved)
  • Security
    • Security issues addressed
    • Threat Model updated
  • Documentation
    • Docs updated (runbook, READMEs)
  • Testing
    • Feature tested on relevant environments
  • Comms
    • Relevant parties notified of changes

Testing

  • Manual Testing required?
    • Local (Ready-For-Test, Local)
    • Test (Ready-For-Test, Test)
    • Preview (Ready-For-Test, Preview)
    • Live (Ready-For-Test, Live)
  • Manual Testing complete?
    • Local
    • Test
    • Preview
    • Live

Additional Testing Steps

  1. List the steps required to test this PR.

Useful Links

@HarveyPeachey HarveyPeachey self-assigned this Apr 15, 2025
'**/tz/**',
'index.stories.jsx',
'index.amp.stories.jsx',
'.storybook/**/*',
Copy link
Contributor Author

@HarveyPeachey HarveyPeachey Apr 16, 2025

Choose a reason for hiding this comment

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

For @typescript-eslint/prefer-optional-chain to work correctly without erroring, I've had to disable linting of anything in .storybook, this is because the tsconfig hasn't been pointed to .storybook, when I added it it surfaced a lot of typescript errors, too many to fix in this PR.

So we either need a follow up ticket and PR to fix the TS errors in .storybook and reinstate linting of the folder, or we don't use this new rule until we do those fixes. Either way we need a ticket to fix the TS errors in .storybook.

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 fine - the components within the storybook directory are mainly for Component Health, and are written in TS. For me, they are much less important than actual components which we use the Simorgh app. Do we even need a follow up ticket to fix this in the future? No harm in creating one, I suppose, but it's low priority.

@HarveyPeachey HarveyPeachey changed the title enables no-unsafe-optional-chaining rule WORLDSERVICE-225: Adds linting rules and standards around optional chaining Apr 16, 2025
@HarveyPeachey HarveyPeachey marked this pull request as ready for review April 16, 2025 09:06
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've had to revert these back to jsx, due to typescript excluding the legacy folder

Copy link
Contributor

@karinathomasbbc karinathomasbbc left a comment

Choose a reason for hiding this comment

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

Given that we had to rename some files from tsx -> jsx, let's create a follow up ticket to fix those. I would also create a separate ticket to fix the TS linting errors within the .storybook directory, which is a lower priority.

'**/tz/**',
'index.stories.jsx',
'index.amp.stories.jsx',
'.storybook/**/*',
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 fine - the components within the storybook directory are mainly for Component Health, and are written in TS. For me, they are much less important than actual components which we use the Simorgh app. Do we even need a follow up ticket to fix this in the future? No harm in creating one, I suppose, but it's low priority.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did this get renamed to jsx to avoid TS linting errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As eslint targets the legacy folder, it will run all linting rules against it, including ts linting rules, but because we've excluded the legacy folder from being targeted by the ts compiler, eslint produces an error. These components should really be fully migrated over

@HarveyPeachey HarveyPeachey merged commit ca5f169 into latest Apr 16, 2025
11 checks passed
@HarveyPeachey HarveyPeachey deleted the WORLDSERVICE-225-optional-chaining-standard branch April 16, 2025 12:14
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.

3 participants