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

Feature/65543 upgrade eslint to v9 #2975

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

GuyHarwood
Copy link
Collaborator

@GuyHarwood GuyHarwood commented Nov 15, 2024

  • upgrade eslint from 8 > 9
  • remove standard
  • remove deprecated plugins

@GuyHarwood GuyHarwood self-assigned this Nov 15, 2024
@GuyHarwood
Copy link
Collaborator Author

closing until all lint issues fixed to avoid pointless CI work

@GuyHarwood GuyHarwood closed this Nov 15, 2024
@GuyHarwood
Copy link
Collaborator Author

core linter rules passing

@GuyHarwood GuyHarwood reopened this Nov 19, 2024
@GuyHarwood
Copy link
Collaborator Author

GuyHarwood commented Dec 31, 2024

exploring prettier...

dev/mtc/admin ❯❯❯ npx prettier . --write --no-semi --single-quote --trailing-comma none

produces 501~ file changes on just the admin app.

will leave for now, pending discussion with wider team.

@GuyHarwood GuyHarwood marked this pull request as ready for review December 31, 2024 11:38
@GuyHarwood GuyHarwood added the CI Enabled Travis will only build PRs with this label assigned label Dec 31, 2024
@GuyHarwood GuyHarwood requested a review from jon-shipley January 3, 2025 12:51
Copy link
Collaborator

@jon-shipley jon-shipley left a comment

Choose a reason for hiding this comment

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

👍 Fantastic work! Adding comments.

You might want to consider whether we want the additional testing burden of modernising (var => consts) the frontend code. This code might be minified, but it is not transpiled, and is delivered as-is which means these changes could affect anyone still using IE11/10 or Safari 10 (or earlier).

admin/delete-this-old-eslint-v8.txt Outdated Show resolved Hide resolved
tslib/old.eslintrc.json Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
import config from '../config'
import { isNil } from 'ramda'
const appInsights = require('applicationinsights')
import * as appInsights from 'applicationinsights'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alarm bells are ringing here. Please do test this. Application Insights is very sensitive to ejs / cjs differences and although we are writing typescript esm, we are compiling to cjs to be run on node.

@@ -40,7 +40,7 @@ export interface IRedisService {
* @param {Array<string>} keys an array of keys to invalidate
* @returns {Promise<Array<[error: Error | null, result: unknown]> | null | undefined>}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The jsdoc is out of date now the call sig has changed. Please consider either updating or deleting the jsdoc.

@@ -25,7 +25,7 @@ export interface IRedisService {
* @param {Array<string>} keys an array of keys to invalidate
* @returns {Promise<void>}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The jsdoc type and the typescript types are misaligned. You can probably delete the jsdoc for *.ts files.

@GuyHarwood
Copy link
Collaborator Author

@jon-shipley based on our SPA browser policy and the govuk guidance we are ok to update the browser JS...

for...of browser compatibility is OK - https://caniuse.com/?search=for...of
const browser compatibility is OK - https://caniuse.com/const

Copy link
Collaborator

@jon-shipley jon-shipley left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Enabled Travis will only build PRs with this label assigned
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants