#6526: Make more background APIs strict (strictNullChecks) #7667

fregante · 2024-02-18T18:56:21Z

What does this PR do?

Checklist

Designate a primary reviewer: @grahamlangford

fregante · 2024-02-18T19:21:32Z

src/auth/authTypes.ts

@@ -50,11 +51,11 @@ export type UserData = Partial<{
  /**
   * The user's primary organization.
   */
-  organizationId: UUID;
+  organizationId: UUID | null;


These types are already inside a Partial<> that set them to X | undefined, so this effectively doesn't change anything at runtime.

fregante · 2024-02-18T19:22:21Z

src/auth/authTypes.ts

  /**
   * The partner principals. Currently, just the Automation Anywhere Control Room principal if applicable.
   * @since 1.7.16
   */
-  readonly partnerPrincipals?: Me["partner_principals"];
+  readonly partnerPrincipals: Me["partner_principals"];


Already part of Partial. ? is implied

fregante · 2024-02-18T19:23:05Z

src/auth/authUtils.ts

-    organizationId: organization?.id,
-    telemetryOrganizationId: telemetryOrganization?.id,
+    organizationId: organization?.id ?? null,
+    telemetryOrganizationId: telemetryOrganization?.id ?? null,


Could not figure out why undefined is not accepted in this place, so I just made null explicit.

UserDataUpdate wraps UserData in Required<>. Apparently that doesn't just strip ? but also prevents setting the var to undefined while still allowing null so long as null is included in the typing. I created a simplified reproduction in TS Playground to confirm.

Given:

type Foo = { a?: string | null | undefined; b?: number; } type Bar = Required<Foo>;

This throws a ts error:

const baz: Bar = { a: undefined, b: 0, }

This doesn't:

const baz: Bar = { a: null, b: 0, }

TIL

Good find. Indeed, because "null is a value", so "baz.a is set"

So our use of Required is meaningless here, i.e. ?? null is only hiding the NPE.

We'll have to use a stricter version of Required:

https://stackoverflow.com/a/75616425/288906

https://stackoverflow.com/a/53050575/288906

Will mention this in #7725

fregante · 2024-02-18T19:23:36Z

src/auth/token.ts

@@ -160,7 +160,7 @@ export async function isLinked(): Promise<boolean> {
 * Return non-sensitive PixieBrix user profile data.
 * @see getExtensionAuth
 */
-export async function getUserData(): Promise<Partial<UserData>> {
+export async function getUserData(): Promise<UserData> {


UserData is already Partial, so I made this change everywhere else.

fregante · 2024-02-18T19:24:01Z

src/background/deploymentUpdater.ts

@@ -18,7 +18,7 @@
 import { type Deployment, type Me } from "@/types/contract";
 import { isEmpty, partition } from "lodash";
 import reportError from "@/telemetry/reportError";
-import { getUID } from "@/background/messenger/api";
+import { getUID } from "@/telemetry/telemetryHelpers";


No need to go through messenger. Allow direct access of getUID from anywhere since it's storage-based. Currently it's only called by the background page and the deployments page (which probably won't be reached before this is generated and stored)

fregante · 2024-02-18T19:24:36Z

src/background/installer.ts

-import reportEvent from "@/telemetry/reportEvent";
-import { initTelemetry } from "@/background/telemetry";
-import { getUID } from "@/background/messenger/api";
+import { initTelemetry, recordEvent } from "@/background/telemetry";


No need to go through messenger. Allow direct access for recordEvent from the background.

fregante · 2024-02-18T19:28:31Z

src/background/messenger/registration.ts

 import {
  ensureContextMenu,
  preloadContextMenus,
  uninstallContextMenu,
-} from "@/background/contextMenus";
+} from "@/background/contextMenus"; // 300 errors


I added some comments to make it easier to tell which files will take more time to make strict and not waste further time trying and trying again.

The lines with "messenger import" means that they will cause another context's Messenger registration to be required, so they'd have to be fixed first.

src/types/contract.ts

grahamlangford · 2024-02-20T14:49:40Z

src/auth/authTypes.ts

@@ -150,7 +151,7 @@ export type OrganizationAuthState = {
  /**
   * The package scope of the organization, or null if not set.
   */
-  readonly scope?: string;
+  readonly scope?: string | null;


I think I've brought this up before, but I don't remember what we decided.

Functionally type foo?: string | null is different from type foo: string | null | undefined. (<key> in <object> behaves differently for instance)

Do we think it's worth the effort to enforce the difference with type checking? There's a tsconfig option: https://www.typescriptlang.org/tsconfig#exactOptionalPropertyTypes

I can try enabling exactOptionalPropertyTypes in a future PR if you'd like to see what that entails. There are "only" 100 errors right now, and it only applies to the strictNullCheck'd files.

Functionally type foo?: string | null is different from type foo: string | null | undefined. (<key> in <object> behaves differently for instance)

I don't particularly care as long as TypeScript handles it correctly (i.e. after key in object, object.key can still be undefined | null).

object.key or object.key == null already covers regular null checking, whereas key in object should only be needed in type guards, e.g. if key in obj, then obj is Foo

The only reason I really think it might be useful is because we've decided to try to treat null and undefined interchangeably as much as possible (hence Nullishable and foo == null vs foo === null).

I can't think of a time it's hurt us not being explicit in the differences. But if it's simple to enforce the distinction, it could potentially make our type guards safer. If it's a pain to implement, I would rather disregard.

Moved to strictNullChecks: enable exactOptionalPropertyTypes #7732

grahamlangford · 2024-02-20T14:59:20Z

src/auth/authUtils.ts

@@ -116,5 +116,5 @@ export function selectExtensionAuthState({
 */
 export async function flagOn(flag: string): Promise<boolean> {
  const authData = await readAuthData();
-  return authData.flags?.includes(flag);
+  return Boolean(authData.flags?.includes(flag));


I'm curious if this is better than

authData.flags?.includes(flag) ?? false

Yeah I like ?? false more. Fixed.

grahamlangford · 2024-02-20T15:04:08Z

src/background/installer.ts

@@ -224,8 +223,11 @@ export async function handleInstall({
  const { version } = browser.runtime.getManifest();

  if (reason === "install") {
-    reportEvent(Events.PIXIEBRIX_INSTALL, {
-      version,
+    void recordEvent({


Is recordEvent wrongly deprecated? Should we remove the deprecation and instead prevent it from being imported outside the background script/service worker?

prevent it from being imported outside the background script/service worker?

That's what noCrossBoundaryImports does but that's not a guarantee due the many many exceptions: pixiebrix/eslint-config-pixiebrix#193 (comment)

All it takes is a single import in @/bricks and it appears in every bundle.

The deprecation makes it "painful" to use this function so you're forced to read its JSDoc. I just updated it to:

/** * @deprecate Only allowed in @/background files. Otherwise use: `import reportEvent from "@/telemetry/reportEvent"` */

If you don't like it at all I can revert the changes to this file and have it go through the messenger anyway.

No, I think your explanation is enough. Just wanted to make sure. We've used deprecations in other places to flag usages we want to warn against even though we don't intend to ever remove the function or type. I think this is a valid example as well.

grahamlangford · 2024-02-20T15:07:31Z

src/background/messenger/api.ts

 // Use this instead: `import reportError from "@/telemetry/reportError"`
 // export const recordError = getNotifier("RECORD_ERROR", bg);


Suggested change

// Use this instead: `import reportError from "@/telemetry/reportError"`

// export const recordError = getNotifier("RECORD_ERROR", bg);

I'd rather keep it there so that nobody thinks we just forgot to add the method. Keeping the full export line also makes it appear when grepping RECORD_ERROR

src/telemetry/telemetryHelpers.ts

fregante · 2024-02-25T09:59:26Z

Ready for review again since #7480 was closed. I opened #7725 to review the Me type

codecov · 2024-02-25T10:06:06Z

Codecov Report

Attention: Patch coverage is 59.01639% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 72.13%. Comparing base (d8bec45) to head (a0ca675).

❗ Current head a0ca675 differs from pull request most recent head 0f5bfdb. Consider uploading reports for the commit 0f5bfdb to get more accurate results

Files	Patch %	Lines
src/telemetry/telemetryHelpers.ts	35.71%	9 Missing ⚠️
src/telemetry/initErrorReporter.ts	0.00%	6 Missing ⚠️
src/background/installer.ts	50.00%	3 Missing ⚠️
src/data/service/errorService.ts	0.00%	3 Missing ⚠️
src/telemetry/logging.ts	57.14%	3 Missing ⚠️
src/telemetry/performance.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7667      +/-   ##
==========================================
- Coverage   72.16%   72.13%   -0.04%     
==========================================
  Files        1267     1267              
  Lines       39691    39701      +10     
  Branches     7359     7368       +9     
==========================================
- Hits        28642    28637       -5     
- Misses      11049    11064      +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/background/telemetry.ts

github-actions · 2024-02-26T15:25:55Z

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

Co-authored-by: Graham Langford <[email protected]>

fregante added 7 commits February 19, 2024 00:45

Direct access to getUID

e205739

Direct access to some background reportEvent

8e079c6

Move 2 methods

4b4ff45

Document effort required for remaining methods

8d9c99f

Second pass

13de533

Third pass

e7436ef

Done

92b528d

vercel bot deployed to Preview February 18, 2024 18:57 View deployment

fregante added 2 commits February 19, 2024 02:49

Mistakes, lint

30d88fb

Strictnullchecks

4c482f9

vercel bot deployed to Preview February 18, 2024 19:51 View deployment

fregante marked this pull request as ready for review February 18, 2024 19:51

fregante commented Feb 18, 2024

View reviewed changes

fregante added the developer experience label Feb 18, 2024

fregante changed the title ~~#6526: Make more background APIs strict~~ #6526: Make more background APIs strict (strictNullChecks) Feb 18, 2024

twschiller assigned fregante Feb 19, 2024

fregante marked this pull request as draft February 19, 2024 06:29

grahamlangford reviewed Feb 20, 2024

View reviewed changes

src/telemetry/telemetryHelpers.ts Outdated Show resolved Hide resolved

fregante added 2 commits February 25, 2024 15:56

Merge remote-tracking branch 'origin/main' into F/types/slice-19

9b6c6e6

Me types adjustments

1d1e212

fregante mentioned this pull request Feb 25, 2024

#7169 Implement transformer for /api/me #7480

Closed

5 tasks

vercel bot deployed to Preview February 25, 2024 08:47 View deployment

Lint

bc4f4cf

vercel bot deployed to Preview February 25, 2024 09:24 View deployment

fregante added 2 commits February 25, 2024 17:54

s/uid/uuid/

8af3297

Update mock

e247cb8

fregante marked this pull request as ready for review February 25, 2024 09:58

fregante requested a review from grahamlangford February 25, 2024 09:58

vercel bot deployed to Preview February 25, 2024 09:59 View deployment

grahamlangford reviewed Feb 26, 2024

View reviewed changes

src/background/telemetry.ts Outdated Show resolved Hide resolved

grahamlangford approved these changes Feb 26, 2024

View reviewed changes

fregante and others added 2 commits February 27, 2024 00:15

Typo

40160c7

Co-authored-by: Graham Langford <[email protected]>

Merge branch 'main' into F/types/slice-19

070f984

fregante enabled auto-merge (squash) February 26, 2024 16:16

vercel bot deployed to Preview February 26, 2024 16:18 View deployment

fregante mentioned this pull request Feb 26, 2024

strictNullChecks: enable exactOptionalPropertyTypes #7732

Closed

Fix lint from other PR

a0ca675

vercel bot deployed to Preview February 26, 2024 16:50 View deployment

/2

0f5bfdb

vercel bot deployed to Preview February 26, 2024 16:58 View deployment

fregante merged commit 4bfba3f into main Feb 26, 2024
14 checks passed

fregante deleted the F/types/slice-19 branch February 26, 2024 17:04

fregante mentioned this pull request Feb 28, 2024

strictNullChecks: ShadowRoot, navigateTab, quickBarApp, auto-add #7751

Merged

1 task

grahamlangford added this to the 1.8.10 milestone Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#6526: Make more background APIs strict (strictNullChecks) #7667

#6526: Make more background APIs strict (strictNullChecks) #7667

fregante commented Feb 18, 2024 •

edited

Loading

fregante Feb 18, 2024

fregante Feb 18, 2024

fregante Feb 18, 2024

grahamlangford Feb 20, 2024 •

edited

Loading

fregante Feb 25, 2024 •

edited

Loading

fregante Feb 18, 2024

fregante Feb 18, 2024

fregante Feb 18, 2024

fregante Feb 18, 2024

grahamlangford Feb 20, 2024

fregante Feb 25, 2024 •

edited

Loading

grahamlangford Feb 26, 2024

fregante Feb 26, 2024

grahamlangford Feb 20, 2024

fregante Feb 25, 2024

grahamlangford Feb 20, 2024

fregante Feb 25, 2024

grahamlangford Feb 26, 2024

grahamlangford Feb 20, 2024

fregante Feb 25, 2024 •

edited

Loading

fregante commented Feb 25, 2024

codecov bot commented Feb 25, 2024 •

edited

Loading

github-actions bot commented Feb 26, 2024

		// Use this instead: `import reportError from "@/telemetry/reportError"`
		// export const recordError = getNotifier("RECORD_ERROR", bg);

#6526: Make more background APIs strict (strictNullChecks) #7667

#6526: Make more background APIs strict (strictNullChecks) #7667

Conversation

fregante commented Feb 18, 2024 • edited Loading

What does this PR do?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamlangford Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

fregante Feb 25, 2024 • 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

fregante Feb 25, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

fregante commented Feb 25, 2024

codecov bot commented Feb 25, 2024 • edited Loading

Codecov Report

github-actions bot commented Feb 26, 2024

fregante commented Feb 18, 2024 •

edited

Loading

grahamlangford Feb 20, 2024 •

edited

Loading

fregante Feb 25, 2024 •

edited

Loading

fregante Feb 25, 2024 •

edited

Loading

fregante Feb 25, 2024 •

edited

Loading

codecov bot commented Feb 25, 2024 •

edited

Loading