Increase typescript-eslint coverage #5622

flozia · 2025-02-12T18:11:33Z

Description

Increases TSLint coverage to include the following files:

src/app/global-error.js
src/db/migrations/*.js
src/scripts/build/*.js
src/scripts/loadtest/*.js

How to test

Running npm run lint should be successful.

…ocations.js

…tions.js

…gnment.js

github-actions · 2025-02-12T18:33:16Z

Preview URL 🚀 : https://blurts-server-pr-5622-mgjlpikfea-uk.a.run.app

rhelmer

This is great, thanks! A few questions and suggestions. It's nice that we can use JSDoc-style comments to shore up the older code like migrations.

rhelmer · 2025-02-12T19:15:14Z

src/db/migrations/20221026215921_add-breaches-table.js

    table.string("title").default("");
+    // @ts-ignore TODO: Determine if the following line should be changed to
+    // `defaultTo` or remain unchanged as initially deployed.
    table.string("domain").default("");


@flozia was this renamed on the knex side? I only see defaultTo in their docs, I don't see any reference to default

I don’t think so — at least I couldn’t find any reference. I was just being cautious about updating existing migration files. Sounds like maybe we should.

Unless we're nuking our prod database (please don't :P) I think it's better to have the migration files accurately reflect how the prod DB got to its current state.

Yes, that is why I’ve just added the ignores. What do you think @rhelmer and @mansaj about leaving those as is vs. updating them? The ones I’ve applied would be:

table.default() -> table.defaultTo()

table.dropUnique("") -> table.dropUnique([""])

… and table.varchar() we may just also leave as is.

I agree that these modifications to the migration files shouldn't mutate the database, mostly I'm just wondering wtf .default() does because it doesn't seem to be documented :)

FWIW, if we want to ensure that changes to existing migration files don't change the schema, we could test it locally:

create new db, run "before" migrations, dump schema (pgdump)

apply changes to migration scripts

repeat step 1 and diff the "before"/"after" schema

It's possible for migration scripts to insert/delete data too but I don't think we tend to do that.

rhelmer · 2025-02-12T19:15:57Z

src/db/migrations/20230703010356_add-onerep-profiles-table.js

 export function up(knex) {
  return knex.schema.createTable("onerep_profiles", (table) => {
    table.increments("id").primary();
    table
      .integer("onerep_profile_id")
      .references("subscribers.onerep_profile_id")
      .nullable();
+    // @ts-ignore TODO: Determine if the following line should be changed


Curious why this doesn't work, varchar should be valid no?

It looks like varchar is implemented, but missing from the type TableBuilder. I think it is recommended to use .string().

Ah OK, yeah "under the hood" in Postgres I believe TEXT and VARCHAR types are implemented the same way so .text() is probably what you want https://knexjs.org/guide/schema-builder.html#text

src/db/migrations/20230907143204_add_onerep_scanresults_table.js

src/db/migrations/20231102024624_add_unique_index_to_scan_results_id.js

src/db/migrations/20240423150332_add_suffix_and_middle_name_to_onerep_profile_table.js

rhelmer · 2025-02-12T19:19:29Z

src/scripts/build/getAutoCompleteLocations.js

+    // @ts-ignore If body is `null` we are already throwing an error above.
    await finished(Readable.fromWeb(body).pipe(stream));


Suggested change

// @ts-ignore If body is `null` we are already throwing an error above.

await finished(Readable.fromWeb(body).pipe(stream));

await finished(Readable.fromWeb(body!).pipe(stream));

This is cleaner than ts-ignore and will result in the same thing at runtime I think, but I think the real potential problem it's pointing out is that body may be non-null but still not a ReadableStream. I'm not sure there's much we can do to handle it in this case though, my suggestion should throw a TypeError at runtime which is probably the best we can do.

Since this is a .js file non-null assertion won’t work here. :/

Oh shoot right sorry I forgot, yeah that's fine then. Unfortunate to have to turn it off for the whole line that's all. I guess if you care you could explicitly check this and throw.

Vinnl

Thanks for being so thorough!

src/scripts/build/nimbusTypes.js

src/scripts/build/uploadAutoCompleteLocations.js

Vinnl · 2025-02-13T11:57:13Z

src/scripts/build/uploadAutoCompleteLocations.js

@@ -293,10 +337,30 @@ try {
            return false;
          }

-          return locationDataPopulated.some((location) => {
+          return locationDataRows.some((location) => {


Is this the bug you were referring to? I don't quite understand why we're suddenly looking at locationDataRows instead of at locationDataPopulated like before, just from this diff.

Yes, that’s the one. locationDataRows is the final data we write to the resulting JSON file. For safe storage, it is as small as possible and missing the feature class (which we can look up in locationDataPopulated). This was the lowest-hanging fix to make without having to touch more of the file.

OK, gotcha, thanks! Only remaining thing I don't follow is the impact of the bug: wouldn't it cause no locations to be returned?

Vinnl · 2025-02-13T11:58:56Z

src/scripts/loadtest/hibp.js

    if (result.success !== true) {
      throw new Error(`Non-success result: ${JSON.stringify(result)}`);
    }
  } catch {
+    // @ts-ignore TODO: Add type for result.


This line doesn't refer to result (just to res, but I assume that that one is typed)?

src/scripts/loadtest/hibp.js

rhelmer

lgtm, thanks for slogging through that! I'd say in general you've erred on the safe side rather than changing existing migration code (and risk mutating the schema for new vs. existing DB instances) which seems appropriate.

flozia added 9 commits February 12, 2025 14:35

chore: Run ESLint on src/app/global-error.js

8f477ee

chore: Address typing issues in src/scripts/build/uploadAutoCompleteL…

041dca8

…ocations.js

chore: Address typing issues in src/scripts/build/getAutoCompleteLoca…

104e90b

…tions.js

chore: Address typing issues in src/scripts/build/nimbusTypes.js

1f85a24

chore: Address typing issues in src/scripts/build/gleanTypes.js

8898122

chore: Address typing issues in src/scripts/build/checkNodeVersionAli…

e37990d

…gnment.js

chore: Address typing issues in src/scripts/loadtest/hibp.js

b481338

chore: Address typing issues in src/db/migrations/*.js

858b5eb

fix: Wrong type in nimbusTypes.js

9afb526

flozia mentioned this pull request Feb 12, 2025

Run ESLint on root files #5619

Merged

flozia requested review from Vinnl, rhelmer and mansaj February 12, 2025 18:16

rhelmer requested changes Feb 12, 2025

View reviewed changes

fix: Knex methods and args

e906434

Vinnl approved these changes Feb 13, 2025

View reviewed changes

flozia changed the title ~~Increase TSLint coverage~~ Increase typescript-eslint coverage Feb 13, 2025

flozia self-assigned this Feb 13, 2025

flozia added 3 commits February 13, 2025 16:10

chore: Infer that parsedAlternateNames does not contain null

e189fb1

chore: Update varchar comments

77fdb3f

chore: Update ts-ignore comment in src/scripts/loadtest/hibp.js

e26b088

flozia requested a review from rhelmer February 13, 2025 15:28

rhelmer approved these changes Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase typescript-eslint coverage #5622

Increase typescript-eslint coverage #5622

flozia commented Feb 12, 2025

github-actions bot commented Feb 12, 2025

rhelmer left a comment

rhelmer Feb 12, 2025

flozia Feb 13, 2025

Vinnl Feb 13, 2025

flozia Feb 13, 2025 •

edited

Loading

rhelmer Feb 13, 2025 •

edited

Loading

rhelmer Feb 13, 2025

rhelmer Feb 12, 2025

flozia Feb 13, 2025

rhelmer Feb 13, 2025

rhelmer Feb 12, 2025

rhelmer Feb 12, 2025 •

edited

Loading

flozia Feb 13, 2025

rhelmer Feb 13, 2025

Vinnl left a comment

Vinnl Feb 13, 2025

flozia Feb 13, 2025

Vinnl Feb 13, 2025

Vinnl Feb 13, 2025

rhelmer left a comment

		// @ts-ignore If body is `null` we are already throwing an error above.
		await finished(Readable.fromWeb(body).pipe(stream));

	// @ts-ignore If body is `null` we are already throwing an error above.
	await finished(Readable.fromWeb(body).pipe(stream));
	await finished(Readable.fromWeb(body!).pipe(stream));

Increase typescript-eslint coverage #5622

Are you sure you want to change the base?

Increase typescript-eslint coverage #5622

Conversation

flozia commented Feb 12, 2025

Description

How to test

github-actions bot commented Feb 12, 2025

rhelmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flozia Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

rhelmer Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhelmer Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vinnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhelmer left a comment

Choose a reason for hiding this comment

flozia Feb 13, 2025 •

edited

Loading

rhelmer Feb 13, 2025 •

edited

Loading

rhelmer Feb 12, 2025 •

edited

Loading