Perform JSON conversion inside getLocal and setLocal#5328
Conversation
| } | ||
| if (RED.workspaces.count() > 0) { | ||
| const hiddenTabs = JSON.parse(RED.settings.getLocal("hiddenTabs")||"{}"); | ||
| const hiddenTabs = RED.settings.getLocal("hiddenTabs", {}); |
There was a problem hiding this comment.
This was not what I meant. My suggestion was to stop using JSON.parse when not needed
Instead of
hiddenTabs = JSON.parse(RED.settings.getLocal("hiddenTabs") || "[]")you could do something like
const serializedHiddenTabs = RED.settings.getLocal("hiddenTabs");
parsedHiddenTabs = serializedHiddenTabs? JSON.parse(serializedHiddenTabs) ?? undefined;I know the amount of time spent parsing "[]" is irrelavant, and your solution works, but I thought it would be good if we never use JSON.parse unless really necessary. Please, don't think I'm being pedantic with this suggestion. I know targeted node-red hardware have a ton of memory, but because I know that parsing json is quite expensive in memory constrained hardware I learned to avoid using it whenever possible.
|
Changing This must be marked as a breaking change |
|
We shouldn't be making any changes to this API. It will break existing users. |
|
We can make it non-breaking based on the number of arguments. Agree? Note: this API is not documented, therefore it's not public... |
Yes, that could work. function setLocal(key: string, data: any, parse = false): void;
function getLocal(key: string, parse = false): string | object | null;However, my preference would be creating new methods and leave the existing ones as is. function setLocalData(key: string, data: unknown): void;
function getLocalData<T = unknown>(key: string): T | null;
No documentation doesn't mean that it isn't used. For example, I'm using undocumented parameters when registering a new history event. I learned about them because I read the code. |
|
I also use undocumented mechanisms and some internal ones 🫣🤫 By that I mean that we do not guarantee the backward compatibility of everything that is internal. If Nick considers this API to be public (simply not documented), then we will apply backward compatibility. |
3 participants
Types of changes
Proposed changes
Discussed in #5322.
RED.settings.getLocalandRED.settings.setLocalChecklist
npm run testto verify the unit tests pass