#6739: Incremental strict null checks adoption: Slice 10 #6808

fregante · 2023-11-02T16:00:04Z

What does this PR do?

Closes Incremental strict null checks adoption: Slice 10 #6739

Checklist

Designate a primary reviewer: @grahamlangford
I'd like to do some smoke tests before merging

fregante · 2023-11-02T16:02:42Z

package.json

@@ -154,6 +154,7 @@
    "slugify": "^1.6.6",
    "stemmer": "^2.0.1",
    "timezone-mock": "^1.3.6",
+    "uint8array-extras": "^0.5.0",


This PR includes some deduplication around base64/data-url parsing, for which we had like 3 functions.

I merged them into one and started using this package for a safe base64/arraybuffer conversion:

Note: In the browser, do not use globalThis.atob() / globalThis.btoa() because they do not support Unicode. This package does.

fregante · 2023-11-02T16:08:16Z

src/bricks/transformers/temporaryInfo/popoverUtils.ts

@@ -62,47 +62,62 @@ const resizerMap = new WeakMap<HTMLElement, IFrameComponent>();
 function popoverFactory(initialUrl: URL): HTMLElement {
  const $container = $(ensureTooltipsContainer());



Diffs in this file best seen without whitespace changes:

I flipped a condition to allow for a early return

I replaced some logic; This appeases the null checks and we will have to do more of it:

bye .length + .push() + .pop()

hello .pop() + null check

I added the html helper to force Parcel to format the HTML snippet. However I don’t think our version is working correctly so I'll add code-tag

src/bricks/transformers/temporaryInfo/popoverUtils.ts

src/starterBricks/helpers.ts

fregante · 2023-11-02T16:13:57Z

src/starterBricks/helpers.ts


-  if (selectors.length === 0) {
+  if (!nextSelector) {


Again here:

bye length + [x, ...rest]

hello shift() + null check

fregante · 2023-11-02T16:16:31Z

src/starterBricks/helpers.ts

-    label: extension.label,
-    extensionLabel: extension.label,
+    label: extension.label ?? undefined,
+    extensionLabel: extension.label ?? undefined,


This is the consequence of one type having optional properties ({x?: Type}) and the other using {x: Type | null}

I can look into a better solution but I think it'd require changes to some higher type like ModComponent.

Longer term, it might be nice to enforce https://www.typescriptlang.org/tsconfig#exactOptionalPropertyTypes

That plus a linting rule to prefer null could clean a lot of that up.

I don’t think it's practical to enforce specifying every property on an object with null if they're really meant to be optional. If anything, I'd enforce the opposite: use an optional key instead of T | null, because it's much easier to handle

It's my understanding that enforcing exactOptionalPropertyTypes would still allow optional properties, but would prevent us from setting a property to undefined. null should be reserved for when we want to specify that we "know a value doesn't exist" while undefined should mean that the value was never set.

Ah yeah I was answering to the second comment, regarding preferring null, which I read as "Use {x: string | null} instead of {x?: string}"

exactOptionalPropertyTypes is unrelated here because {x?: string} still generates string | undefined regardless of exactOptionalPropertyTypes, so it would still conflict with string | null.

This sort of mismatches are why I subscribe to sindresorhus/meta#7, which also cites this problem.

null can be important. Especially with JSON. See JSON.stringify({ foo: undefined }) vs JSON.stringify({ foo: null })

exactOptionalPropertyTypes is unrelated here because {x?: string} still generates string | undefined regardless of exactOptionalPropertyTypes, so it would still conflict with string | null
Is that true? It would definitely still conflict with string | null, but {x: undefined} would be flagged. exactOptionalPropertyTypes would allow { } but error on {x: undefined}.

To allow { x: undefined }, we'd have to specify { x?: string | undefined }, which we could then flag with a linting rule.

json.foo is still nullish, it's not important if you consider null and undefined the same.

JSON.stringify([undefined, 1]) will generate null in cases where there was undefined, but then again if you treat both the same way it doesn't matter if JSON.parse() generate [null, 1] and the expected type is Array<number | undefined>

JSON.stringify([undefined, 1]) will generate null in cases where there was undefined
TIL

fregante · 2023-11-02T16:17:54Z

src/starterBricks/menuItemExtension.ts

-    this.cancelListeners.add(() => {
-      mutationObserver.disconnect();
-    });
+    onAbort(this.cancelController, mutationObserver);


This helper also comes from Refined GitHub. AbortController/AbortSignal has bad DX, but this makes it much nicer.

fregante · 2023-11-02T16:19:06Z

src/testUtils/testAfterEnv.ts

-// https://stackoverflow.com/q/52612122/288906
-// globalThis.crypto = {
-//   getRandomValues: (array) => crypto.randomBytes(array.length),
-// };


This was required on Node 16, but it already exists in Node 18. We'll never need it again.

fregante · 2023-11-02T16:21:11Z

src/utils/parseDataUrl.ts

+  return new Blob([ia], { type: mimeTypeEssence });
+}
+
+export { convertDataUrl };


TODO:

Add tests for convertDataUrl

Our parseDataUrl is already tested and so is the 3rd party base64ToUint8Array

src/utils/promiseUtils.ts

codecov · 2023-11-03T07:01:15Z

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (05da7ae) 70.39% compared to head (5a18c18) 70.41%.
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6808      +/-   ##
==========================================
+ Coverage   70.39%   70.41%   +0.02%     
==========================================
  Files        1193     1193              
  Lines       37053    37049       -4     
  Branches     6935     6949      +14     
==========================================
+ Hits        26082    26087       +5     
+ Misses      10971    10962       -9

Files	Coverage Δ
src/auth/authTypes.ts	`100.00% <ø> (ø)`
src/bricks/effects/clipboard.ts	`85.71% <100.00%> (+0.52%)`	⬆️
src/components/fields/fieldUtils.ts	`83.05% <100.00%> (ø)`
src/contentScript/sidebarController.tsx	`37.71% <ø> (ø)`
src/types/bricks/readerTypes.ts	`75.00% <100.00%> (ø)`
src/utils/clipboardUtils.ts	`72.34% <ø> (-4.03%)`	⬇️
src/utils/domUtils.ts	`81.39% <ø> (-1.94%)`	⬇️
src/bricks/readers/ImageExifReader.ts	`29.03% <50.00%> (+8.51%)`	⬆️
src/utils/parseDataUrl.ts	`86.84% <86.66%> (-1.16%)`	⬇️
src/utils/promiseUtils.ts	`73.91% <90.00%> (+4.46%)`	⬆️
... and 5 more

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

fregante

I reviewed the code multiple times and tested the changed logic. Should be good to go.

fregante · 2023-11-03T05:07:40Z

src/bricks/effects/clipboard.ts

@@ -86,7 +86,7 @@ export class CopyToClipboard extends EffectABC {
        }

        try {
-          blob = dataURItoBlob(text);
+          blob = convertDataUrl(text, "Blob");


Conversion tested via:

const image = new Image() image.src = URL.createObjectURL(blob); document.body.prepend(image);

But the brick is otherwise broken on main:

Clipboard brick: Failed to read or decode Blob for clipboard item type image/png. #6812

fregante · 2023-11-03T05:09:08Z

src/bricks/readers/ImageExifReader.ts

 async function getData(img: HTMLImageElement): Promise<ArrayBuffer> {
  // Adapted from https://github.com/exif-js/exif-js/blob/master/exif.js#L384
  if (/^data:/i.test(img.src)) {
    // Data URI
-    return base64ToArrayBuffer(img.src);
+    return convertDataUrl(img.src, "ArrayBuffer");


Unable to test this brick, I saw this message with two different bricks. What am I doing wrong?

In convertDataUrl, arraybuffer comes before blob in the code, so we can assume this also works.

@BLoe or @twschiller, I've not used this brick. Do you have any context?

I think that's a super old brick, I don't know if anyone uses it. Looks like both image bricks are element-target bricks, where you don't explicitly pass an input, but you need to use them with element target mode. If you ask on Slack someone can probably link you to the PB developer docs on how to use them.

I was able to test it.
Steps:

Navigate to https://github.com/ianare/exif-samples

Create a Context Menu starter brick

Add the EXIF brick

Under Advanced Options, set Target Root Mode to Element

Set Target Element to @input.element.ref

fregante · 2023-11-03T07:04:59Z

src/starterBricks/helpers.ts

-// TODO: replace callback with having caller pass AbortSignal
-export function onNodeRemoved(node: Node, callback: () => void): () => void {
-  const ancestors = getAncestors(node);
+export function onNodeRemoved(


Greatly simplified, now uses a single MutationObserver, no sets/weaksets, no nulls.

fregante · 2023-11-03T09:36:59Z

package.json

@@ -237,6 +238,7 @@
    "@types/webpack-env": "^1.18.3",
    "@types/whatwg-mimetype": "^3.0.1",
    "axios-mock-adapter": "^1.22.0",
+    "blob-polyfill": "^7.0.20220408",


Jest 🤷‍♂️

jsdom/jsdom#2555 (comment)

fregante · 2023-11-03T09:41:14Z

src/starterBricks/panelExtension.ts


  private uninstalled = false;

+  // TODO: Rewrite this logic to use AbortController and to possibly unify the logic with the
+  // global cancelController. The `onAbort` utility might be useful to link multiple controllers
+  // together, but it's probably best to skip the duplicate `cancelController`.
  private readonly cancelRemovalMonitor: Map<string, () => void>;


This file has a weird setup where there's a main "cancel callbacks" Set and also a per-extension Map:

The main Set is currently used, I turned it into an AbortController

The Map is currently already commented out, so I left some context for the changes required.

grahamlangford · 2023-11-06T15:31:31Z

src/utils/promiseUtils.test.ts

@@ -145,3 +146,89 @@ describe("asyncMapValues", () => {
    });
  });
 });
+
+describe("onAbort", () => {


github-actions · 2023-11-08T14:04:34Z

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.

fregante added 3 commits November 2, 2023 22:47

Pass 1

7fd21aa

Pass 2

af92719

Pass 3

21727ee

fregante commented Nov 2, 2023

View reviewed changes

fregante assigned fregante and grahamlangford Nov 2, 2023

fregante mentioned this pull request Nov 3, 2023

Clipboard brick: Failed to read or decode Blob for clipboard item type image/png. #6812

Open

fregante added 2 commits November 3, 2023 14:08

Add temporaryPanelProtocol

730e123

Just use MutationObserver

60f5ce9

vercel bot deployed to Preview November 3, 2023 06:55 View deployment

panelExtension abortController comment

02956ba

vercel bot deployed to Preview November 3, 2023 07:27 View deployment

fregante added 2 commits November 3, 2023 16:01

Test parseDataUrl and convertDataUrl

275fe21

Test onAbort

5798d94

vercel bot deployed to Preview November 3, 2023 09:36 View deployment

Use code-tag

94cf919

vercel bot deployed to Preview November 3, 2023 09:48 View deployment

fregante commented Nov 3, 2023

View reviewed changes

vercel bot deployed to Preview November 4, 2023 05:30 View deployment

fregante force-pushed the F/types/slice-10 branch from e3de789 to 94cf919 Compare November 4, 2023 07:13

grahamlangford reviewed Nov 6, 2023

View reviewed changes

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

ec5e0d5

vercel bot deployed to Preview November 6, 2023 16:21 View deployment

Update popoverUtils.ts

6953f33

vercel bot deployed to Preview November 6, 2023 16:32 View deployment

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

5a18c18

vercel bot deployed to Preview November 7, 2023 01:47 View deployment

grahamlangford approved these changes Nov 8, 2023

View reviewed changes

fregante merged commit 4f4c027 into main Nov 9, 2023
20 checks passed

fregante deleted the F/types/slice-10 branch November 9, 2023 01:28

fregante mentioned this pull request Nov 9, 2023

#6739: Incremental strict null checks adoption: Slice 11 #6847

Merged

1 task

grahamlangford added this to the 1.8.3 milestone Nov 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#6739: Incremental strict null checks adoption: Slice 10 #6808

#6739: Incremental strict null checks adoption: Slice 10 #6808

fregante commented Nov 2, 2023 •

edited

fregante Nov 2, 2023

fregante Nov 2, 2023 •

edited

fregante Nov 2, 2023

fregante Nov 2, 2023

grahamlangford Nov 2, 2023

grahamlangford Nov 2, 2023

fregante Nov 3, 2023 •

edited

grahamlangford Nov 6, 2023

fregante Nov 6, 2023

grahamlangford Nov 6, 2023

grahamlangford Nov 6, 2023

fregante Nov 6, 2023

grahamlangford Nov 8, 2023

fregante Nov 2, 2023

fregante Nov 2, 2023

fregante Nov 2, 2023 •

edited

codecov bot commented Nov 3, 2023 •

edited

fregante left a comment

fregante Nov 3, 2023

fregante Nov 3, 2023

grahamlangford Nov 6, 2023

BLoe Nov 6, 2023

grahamlangford Nov 8, 2023

fregante Nov 3, 2023

fregante Nov 3, 2023

fregante Nov 3, 2023

grahamlangford Nov 6, 2023

github-actions bot commented Nov 8, 2023

		@@ -62,47 +62,62 @@ const resizerMap = new WeakMap<HTMLElement, IFrameComponent>();
		function popoverFactory(initialUrl: URL): HTMLElement {
		const $container = $(ensureTooltipsContainer());

#6739: Incremental strict null checks adoption: Slice 10 #6808

#6739: Incremental strict null checks adoption: Slice 10 #6808

Conversation

fregante commented Nov 2, 2023 • edited

What does this PR do?

Checklist

Choose a reason for hiding this comment

fregante Nov 2, 2023 • edited

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 Nov 3, 2023 • edited

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 Nov 2, 2023 • edited

Choose a reason for hiding this comment

codecov bot commented Nov 3, 2023 • edited

Codecov Report

fregante 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

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

github-actions bot commented Nov 8, 2023

fregante commented Nov 2, 2023 •

edited

fregante Nov 2, 2023 •

edited

fregante Nov 3, 2023 •

edited

fregante Nov 2, 2023 •

edited

codecov bot commented Nov 3, 2023 •

edited