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

Replace most uses of null with undefined #667

Merged
merged 4 commits into from
Dec 6, 2023
Merged

Conversation

lyonsil
Copy link
Member

@lyonsil lyonsil commented Dec 5, 2023

This change is Reviewable

@lyonsil lyonsil linked an issue Dec 5, 2023 that may be closed by this pull request
19 tasks
@lyonsil lyonsil force-pushed the 642-default-parameters branch from 2a6b614 to 1075281 Compare December 5, 2023 17:01
Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for the hard work on all this :)

Reviewed 46 of 46 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @lyonsil)


src/extension-host/services/extension.service.ts line 762 at r1 (raw file):

  // Save extensions that have JavaScript to run
  // If main is am empty string, having no JavaScript is intentional. Do not load this extension

typo: an


src/renderer/hooks/papi-hooks/use-promise.hook.ts line 44 at r1 (raw file):

        if (promiseIsCurrent) {
          // If the promise returned undefined, it purposely did this to do nothing. Maybe its dependencies are not set up
          if (result !== undefined) setValue(() => result);

Sorry, I didn't clearly convey my proposed direction for this hook. My proposition was to remove this and always set it instead of having some condition in which it doesn't set it. It seemed like this condition would probably feel more like a bug than a feature if it's on the only "not defined"-like value you can use in the codebase. We can definitely just add a bullet point to consider as part of #552 instead if that's preferable; no rush.


src/shared/services/settings.service.ts line 71 at r1 (raw file):
I don't actually know how this worked before without as unknown as... Can you change this line and another line above with the following to avoid the as unknown as? Those are best to avoid in general if we can:

emitter as PapiEventEmitter<SettingTypes[SettingNames] | undefined>

and change the type of onDidUpdateSettingEmitters to the following:

const onDidUpdateSettingEmitters = new Map<
  SettingNames,
  PapiEventEmitter<SettingTypes[SettingNames] | undefined>
>();

I think onDidUpdateSettingEmitters has the wrong type as is. I understand this isn't your problem, but we generally explain why we have as unknown as. I think these quick tweaks would hopefully prevent needing to put it at all.

I would vote that we put it in our style guide to require an explanation for as unknown as since it effectively completely destroys type checking, but I dunno if we have ever really conclusively stated we should do that.


src/shared/utils/papi-util.ts line 162 at r1 (raw file):

// Something to stand for both "undefined" and "null"
const NIL_MONIKER: string = '__NIL__';

Sorry it wasn't super clear in my description, but I believe we should use null instead of a special moniker. That would express better in the JSON (no weird magic values; it's just normal ol' JSON) and not require additional logic in other languages that don't have a difference between null and undefined. Seems a bit nicer in my mind than requiring everyone who uses the WebSocket to write a custom serializer and deserializer over this little thing. Would be glad to hear your opinion and discuss if you have a different perspective - not sure if you came to a different conclusion after considering benefits and drawbacks or just thought I was suggesting this.

Honestly I'm surprised everything works with the C# right now because of this 🤔


src/shared/utils/papi-util.test.ts line 328 at r1 (raw file):

  });

  it('successfully determines object with `undefined` prop is not serializable', () => {

Why did these two tests flip? I think it makes sense to consider these to be serializable, especially since even undefined is now serializable. I consider this to be a bug currently.

If this is some crazy 4d chess stuff because of the change, maybe we can file an issue and keep moving with our lives for now - would need to consider the potential impacts of this bug. But it would be good to mark these tests as not performing correctly and link the issue (as in the test below - my most ceremonious UNsuccessfully haha) for now if we skip on fixing this.

irahopkinson
irahopkinson previously approved these changes Dec 5, 2023
Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

:lgtm: thanks for this work - loving it. My comments are all non-blocking

Reviewed 44 of 46 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @lyonsil)


src/renderer/components/web-view.component.tsx line 36 at r3 (raw file):

  // This ref will always be defined
  // eslint-disable-next-line no-type-assertion/no-type-assertion
  const iframeRef = useRef<HTMLIFrameElement>(undefined!);

BTW can you use undefined with useRef? In another file you add a comment that indicates that you can't.


src/renderer/context/papi-context/test.context.ts line 3 at r3 (raw file):

import { createContext } from 'react';

const TestContext = createContext<string>('');

BTW strictly now it doesn't need the type specifier, i.e const TestContext = createContext('');, cleaner is better here, IMO.

Code quote:

<string>

src/shared/utils/papi-util.test.ts line 37 at r3 (raw file):

  });

  it('can deserialize with more than one separator in request types', () => {

BTW Adding each of these to the it descriptions indicates to me these series of it blocks should be moved to a nested describe block.

Code quote:

 in request types

src/shared/utils/papi-util.test.ts line 210 at r3 (raw file):

});

describe('serialize and deserialize', () => {

BTW descriptions are typically in Title Case to allow them to be distinguished in the output.

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @irahopkinson and @lyonsil)


src/renderer/components/web-view.component.tsx line 36 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW can you use undefined with useRef? In another file you add a comment that indicates that you can't.

I think maybe the issue with the other one is that it was being compared to null in some spot (I think in our code, but it may also have been related to rc-dock. I didn't look super closely). I think undefined is probably fine in both places if it doesn't go into any rc-dock apis.

Copy link
Member Author

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tjcouch-sil)


src/extension-host/services/extension.service.ts line 762 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

typo: an

Fixed


src/renderer/components/web-view.component.tsx line 36 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW can you use undefined with useRef? In another file you add a comment that indicates that you can't.

Good catch - I made the other comment based on something I read from someone else's post online, but when I tested it later it worked fine. I forgot to change to other one to undefined afterwards.


src/renderer/context/papi-context/test.context.ts line 3 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW strictly now it doesn't need the type specifier, i.e const TestContext = createContext('');, cleaner is better here, IMO.

Good point - Fixed.


src/renderer/hooks/papi-hooks/use-promise.hook.ts line 44 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Sorry, I didn't clearly convey my proposed direction for this hook. My proposition was to remove this and always set it instead of having some condition in which it doesn't set it. It seemed like this condition would probably feel more like a bug than a feature if it's on the only "not defined"-like value you can use in the codebase. We can definitely just add a bullet point to consider as part of #552 instead if that's preferable; no rush.

Got it - sorry for the confusion. Done.


src/shared/services/settings.service.ts line 71 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I don't actually know how this worked before without as unknown as... Can you change this line and another line above with the following to avoid the as unknown as? Those are best to avoid in general if we can:

emitter as PapiEventEmitter<SettingTypes[SettingNames] | undefined>

and change the type of onDidUpdateSettingEmitters to the following:

const onDidUpdateSettingEmitters = new Map<
  SettingNames,
  PapiEventEmitter<SettingTypes[SettingNames] | undefined>
>();

I think onDidUpdateSettingEmitters has the wrong type as is. I understand this isn't your problem, but we generally explain why we have as unknown as. I think these quick tweaks would hopefully prevent needing to put it at all.

I would vote that we put it in our style guide to require an explanation for as unknown as since it effectively completely destroys type checking, but I dunno if we have ever really conclusively stated we should do that.

The code change is done. I'll leave the style guide discussion as a separate thing outside of this PR.


src/shared/utils/papi-util.ts line 162 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Sorry it wasn't super clear in my description, but I believe we should use null instead of a special moniker. That would express better in the JSON (no weird magic values; it's just normal ol' JSON) and not require additional logic in other languages that don't have a difference between null and undefined. Seems a bit nicer in my mind than requiring everyone who uses the WebSocket to write a custom serializer and deserializer over this little thing. Would be glad to hear your opinion and discuss if you have a different perspective - not sure if you came to a different conclusion after considering benefits and drawbacks or just thought I was suggesting this.

Honestly I'm surprised everything works with the C# right now because of this 🤔

Done - I bounced between a few options. Everything is lossy because of the JS language design, so it all feels a little weird. Sorry for the misunderstanding. This probably is better for cross-language compatibility on the websocket.


src/shared/utils/papi-util.test.ts line 328 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Why did these two tests flip? I think it makes sense to consider these to be serializable, especially since even undefined is now serializable. I consider this to be a bug currently.

If this is some crazy 4d chess stuff because of the change, maybe we can file an issue and keep moving with our lives for now - would need to consider the potential impacts of this bug. But it would be good to mark these tests as not performing correctly and link the issue (as in the test below - my most ceremonious UNsuccessfully haha) for now if we skip on fixing this.

They flipped because the behavior changed based on the new definition. I gave an example of this in the JSDocs for serialize and deserialize:

For example, `{ a: 1, b: undefined, c: null }` will become `{ a: 1 }` after passing through serialize then deserialize.

The definition of isSerializable is essentially "is the JSON string representing this object the same if I serialize it once compared to if I serialize it after round-tripping." The answer to that is "no" for undefined and null properties. For the example above, the first time it serializes the string is { a: 1, b: null, c: null }, and the subsequent times will be { a: 1 }. We can change the meaning of isSerializable if you think that definition is the wrong one to use, but I thought this was clear going into this issue.


src/shared/utils/papi-util.test.ts line 37 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW Adding each of these to the it descriptions indicates to me these series of it blocks should be moved to a nested describe block.

The describe block these were in wasn't named in a parallel way with the other describe blocks, so I did some renaming. Note these tests existed prior to this PR.


src/shared/utils/papi-util.test.ts line 210 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW descriptions are typically in Title Case to allow them to be distinguished in the output.

I adjusted all the descriptions to use Title Case except for the function names. I left those to match the actual casing of the functions themselves.

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround on these things! Almost there - just one hopefully small bit of discussion left and one optional tiny thing.

Reviewed 8 of 8 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lyonsil)


src/renderer/hooks/papi-hooks/use-promise.hook.ts line 44 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Got it - sorry for the confusion. Done.

No worries. Thanks!


src/shared/utils/papi-util.ts line 162 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Done - I bounced between a few options. Everything is lossy because of the JS language design, so it all feels a little weird. Sorry for the misunderstanding. This probably is better for cross-language compatibility on the websocket.

Thanks! Yeah, I agree it's weird... Hopefully this will all straighten things out a bit though. Thanks for the great work on it :)


src/shared/utils/papi-util.ts line 162 at r4 (raw file):

/**
 * Converts a JavaScript value to a JSON string, changing `null` and `undefined` values to a moniker

Should it be specified now that this is converting to null instead of a moniker?


src/shared/utils/papi-util.test.ts line 328 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

They flipped because the behavior changed based on the new definition. I gave an example of this in the JSDocs for serialize and deserialize:

For example, `{ a: 1, b: undefined, c: null }` will become `{ a: 1 }` after passing through serialize then deserialize.

The definition of isSerializable is essentially "is the JSON string representing this object the same if I serialize it once compared to if I serialize it after round-tripping." The answer to that is "no" for undefined and null properties. For the example above, the first time it serializes the string is { a: 1, b: null, c: null }, and the subsequent times will be { a: 1 }. We can change the meaning of isSerializable if you think that definition is the wrong one to use, but I thought this was clear going into this issue.

Ah man, I was worried that's what happened here... I'm bummed that it doesn't deserialize back out to { a: 1, b: undefined, c: undefined }, but that's the 4d chess stuff I was thinking of; I guess we would have to stop using a reviver and instead recursively crawl the entries of the object and change all nulls to undefined after JSON.parse runs as it seems the step of removing undefined values happens after the reviver function runs. Bummer :(

In my mind, undefined and not being present should be considered equivalent when determining if it's serializable, so I think that would mean isSerializable is a bit broken. I was hoping the reviver would keep the objects' undefined values around unlike default JSON.parse. But I think it's probably fine to just make an issue and worry about it another time whenever we run into an issue if you want.

I think the only particular consequence right now that I can think of is that people will not be able to make webview state an object that has an undefined member. I guess we can cross that bridge when we get there if you prefer. However, I would like for this test to indicate that this is not expected functionality but has been delayed to another issue (unless you want to discuss that it isn't a bug).

Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lyonsil)


src/shared/utils/papi-util.test.ts line 37 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

The describe block these were in wasn't named in a parallel way with the other describe blocks, so I did some renaming. Note these tests existed prior to this PR.

I did know they were there before (I wrote them) but you had changed their description so that makes it part of this PR. I think your solution is better than my suggestion.

irahopkinson
irahopkinson previously approved these changes Dec 6, 2023
Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lyonsil)

Copy link
Member Author

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 45 of 48 files reviewed, 1 unresolved discussion (waiting on @irahopkinson and @tjcouch-sil)


src/shared/utils/papi-util.ts line 162 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Should it be specified now that this is converting to null instead of a moniker?

Good point - fixed


src/shared/utils/papi-util.test.ts line 328 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Ah man, I was worried that's what happened here... I'm bummed that it doesn't deserialize back out to { a: 1, b: undefined, c: undefined }, but that's the 4d chess stuff I was thinking of; I guess we would have to stop using a reviver and instead recursively crawl the entries of the object and change all nulls to undefined after JSON.parse runs as it seems the step of removing undefined values happens after the reviver function runs. Bummer :(

In my mind, undefined and not being present should be considered equivalent when determining if it's serializable, so I think that would mean isSerializable is a bit broken. I was hoping the reviver would keep the objects' undefined values around unlike default JSON.parse. But I think it's probably fine to just make an issue and worry about it another time whenever we run into an issue if you want.

I think the only particular consequence right now that I can think of is that people will not be able to make webview state an object that has an undefined member. I guess we can cross that bridge when we get there if you prefer. However, I would like for this test to indicate that this is not expected functionality but has been delayed to another issue (unless you want to discuss that it isn't a bug).

Done - comments have been updated, and #672 has been filed to look at in the future.

@lyonsil lyonsil enabled auto-merge (squash) December 6, 2023 00:56
Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


src/shared/utils/papi-util.test.ts line 328 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Done - comments have been updated, and #672 has been filed to look at in the future.

Sounds great. Thank you!

@lyonsil lyonsil merged commit 8dbf02b into main Dec 6, 2023
6 checks passed
@lyonsil lyonsil deleted the 642-default-parameters branch December 6, 2023 16:27
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.

Command and network object default parameters are not respected
3 participants