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

Command and network object default parameters are not respected #642

Closed
19 tasks done
tjcouch-sil opened this issue Nov 14, 2023 · 0 comments · Fixed by #667
Closed
19 tasks done

Command and network object default parameters are not respected #642

tjcouch-sil opened this issue Nov 14, 2023 · 0 comments · Fixed by #667
Assignees
Labels
bug Something isn't working

Comments

@tjcouch-sil
Copy link
Member

tjcouch-sil commented Nov 14, 2023

Describe the bug
When someone registers a command or a network object with a function that has a default value for a parameter, that default value is not honored if undefined is passed as the argument. This is due to the fact that JSON doesn't support undefined, so it's converted to null over the websocket. As such, the parameter thinks the argument is null and therefore doesn't use the default parameter (JavaScript only uses the default parameter if an argument is not provided or is undefined. null doesn't satisfy those conditions).

To Reproduce
Steps to reproduce the behavior:

  1. In web-view.service-host.ts, remove the first few lines that provide a default value for layout if it is not provided (a forced default value)
  2. Open the Resource Viewer (it runs getWebView(stuff, undefined, stuff), so the middle parameter layout is undefined and will be converted to null)
  3. See the error that type doesn't exist on null in Chrome DevTools

Expected behavior
undefined should properly trigger default values on parameters for commands and network object functions (and everything else on the papi; these are just the first examples that came to mind).

In a call on 11/14/23, we decided that we should make a serializer and deserializer function in JS that always revives null to undefined. This is due to null being an object and not triggering default parameters (among other issues - see this article on null for more reasons not to use it). Since we are very significantly interacting with other languages that do not have the distinction of undefined vs null (and JSON doesn't even have it), it seemed best to us to somewhat enforce the same in our JS where possible. We should do the following:

  • Create deserialize and serialize functions in papi-util.ts that we use at the lowest level of our network connectors instead of JSON.stringify and JSON.parse directly that use JSON.stringify with a replacer that converts undefined to null and then JSON.parse with a reviver that converts null to undefined. See this playground for an example of a replacer and a reviver. This is not quite what we are intending to use as our replacer and reviver, but it shows the general idea of what we want to do.
    • These deserialize and serialize functions should mirror the corresponding JSON versions and have the same arguments. If someone passes in a replacer, run that function inside our replacer before doing our conversion between null and undefined. If someone passes in a reviver, run that function inside our reviver after doing our conversion between null and undefined.
    • The JSDocs for these should explain what they do, why we decided to do that, and when to use these functions. These functions should generally be used when interacting with JSON that goes around the papi. Since these functions do not round-trip JSON (meaning they irreversibly modify JSON if you deserialize and serialize) because they change null to undefined, they should not be used in situations where data integrity is critical like with some user data.
  • Update papi-util.ts's isSerializable to the new functions and update its JSDoc accordingly
  • Create unit tests for the serialization functions that make sure that null comes out to undefined within objects and in the middle of arrays. See the playground linked above for some more context on what we're testing
  • Change JSON.stringify to the new papi serialize everywhere in our code
  • Change JSON.parse to the new papi deserialize everywhere in our code except the following location:
    • check-native-dep.js
  • Change our style guide to indicate we always use undefined in JavaScript when we have control over our code unless we need to use null for some reason like interfacing with someone else's API. Note: null is appropriate in languages that do not have both null and undefined such as C# and JSON.
  • Install and enable no-null/no-null eslint rule in core and all extension repos that disallows using null - that way, we have to comment why when we use it
  • Rework all of our JavaScript APIs that use null as a significantly different value than undefined
    • Rework resource-viewer.ts's openResourceViewer (and the resourceViewer.open command) so it returns undefined if the user cancels the dialog
    • Rework text collection's open command so it returns undefined if the user cancels the dialog
    • Rework word list's open command so it returns undefined if the user cancels the dialog
    • Rework all dialog calls so they return undefined instead of null if the user cancels the dialog. Make sure to get all calls to resolveDialogRequest that call null like in platform-dock-layout.component.tsx. Note: you will need to remove the note in the dialog-service.model.ts JSDoc mentioning that null is used so undefined can be a meaningful value. There are also a number of other JSDocs to update like cancelDialog in dialog-base.data.ts
    • Rework snackbar.component.tsx autoHideDuration - default to 0 which would mean "do not autohide", and convert 0 to null internally since that's what the mui snackbar wants
    • Remove null on all our public-facing type signatures. However, unfortunately, the way the data grid component we use internally is typed, you will still have to figure out a way to pass in null on these fields. Maybe you can just made another internal type that extends the current public column type we make that adds null on those specific fields.
    • Currently, in our manifest.json, we use null to indicate that an extension does not and should not have a JavaScript main file to run. We throw an error if main is not specified to help extension developers to understand what they are missing. Change the value to use from null to empty string ''. This will need to be updated in a few manifest.json files, in extension.service.ts, and in webpack.util.ts
    • In use-promise.hook.ts, no longer do the special null return.
    • For settings service and use-settings.hook.ts, use undefined instead of null. Make sure to properly interface with localstorage which can return null but not undefined
    • In client-network-connector.service.ts, in notifyClientConnected, we can change the localstorage call line to const reconnectingClientGuid = localStorage.getItem(CLIENT_GUID_KEY) ?? undefined; (add ?? undefined), we can remove null from reconnectingClientGuid in ClientConnect in network-connector.model.ts and related places
    • Replace all remaining nulls with undefined in JS code unless there would be a problem with an API we don't control if you did so
      • For example, do not replace the nulls in evil.js because they are using document.getElementById which may return null
      • Do not replace nulls in useRef when the ref is being assigned to a component's ref (like useref<HTMLDivElement>(null)) for the same reason as above.
      • For another example, do not change the null in edit-papi-d-ts.ts because RegExp.exec returns null in some cases. There are also other uses of null in relation to RegExp that should be left alone
      • Leave null in ChildProcessByStdio in extension-host.service.ts as that is from an API we are using
      • Leave null in local-storage.polyfill.ts as I think that is a standard way to polyfill
      • Leave null in platform-dock-layout.component.tsx where we are interacting with the dockLayout specifically
      • Leave null in util.ts's isErrorWithMessage since that is specific utility functionality that interfaces with JavaScript APIs

See discussion for more information.

@tjcouch-sil tjcouch-sil added the bug Something isn't working label Nov 14, 2023
@tjcouch-sil tjcouch-sil added this to the 2023-12 Q4 Maintenance milestone Nov 14, 2023
@tjcouch-sil tjcouch-sil added needs design Waiting on further design decisions and clarification and removed needs design Waiting on further design decisions and clarification labels Nov 15, 2023
@lyonsil lyonsil self-assigned this Dec 1, 2023
@lyonsil lyonsil linked a pull request Dec 5, 2023 that will close this issue
@lyonsil lyonsil reopened this Dec 6, 2023
@lyonsil lyonsil closed this as completed Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants