Skip to content

Prevent setting state on unmounted withOnyx components #120

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.

Sign up for GitHub

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

Merged
marcaaron merged 1 commit into from
Feb 25, 2022
Merged

Prevent setting state on unmounted withOnyx components #120

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/Onyx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,12 @@ function keyChanged(key, data) {
* @param {String} key
*/
function sendDataToConnection(config, val, key) {
// If the mapping no longer exists then we should not send any data.
// This means our subscriber disconnected or withOnyx wrapped component unmounted.
if (!callbackToStateMapping[config.connectionID]) {

@roryabraham roryabraham Feb 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is probably just me being confused ... but I don't get how this works, because in connect sendDataToConnection is called with the mapping, which AFAICT does not have a connectionID property 🤔

@marcaaron marcaaron Feb 25, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Gets added in the connect()method.

return;
}

if (config.withOnyxInstance) {
config.withOnyxInstance.setWithOnyxState(config.statePropertyName, val);
} else if (_.isFunction(config.callback)) {
Expand Down Expand Up @@ -388,6 +394,7 @@ function sendDataToConnection(config, val, key) {
function connect(mapping) {
const connectionID = lastConnectionID++;
callbackToStateMapping[connectionID] = mapping;
callbackToStateMapping[connectionID].connectionID = connectionID;

@roryabraham roryabraham Feb 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need to give the mapping itself a connectionID property here? Because it looks like we call sendDataToConnection(mapping), and then in that function try to access config.connectionID, which I think would always be undefined unless I'm missing something?

@marcaaron marcaaron Feb 25, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

callbackToStateMapping[connectionID] === mapping the assignment creates an object reference.

@roryabraham roryabraham Feb 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, so since we do callbackToStateMapping[connectionID] = mapping;, then doing callbackToStateMapping[connectionID].connectionID = connectionID also sets the connectionID on the mapping object? TIL

@marcaaron marcaaron Feb 25, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good overview here: https://eloquentjavascript.net/04_data.html#h_C3n45IkMhg

@kidroca kidroca Mar 7, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mutating input parameters is considered a bad practice, some more strict languages would not even allow it
It might not cause problems here, but someone else can see the code and apply the same in other places
Besides it's possible to achieve the same thing without mutating the input by introducing a local variable

const privateMapping = {
  ...mapping,
  connectionID, 
};

callbackToStateMapping[connectionID] = privateMapping;

Also because of referential equality it should be possible to achieve the same without capturing connectionID

    // If the mapping no longer exists then we should not send any data.
    // This means our subscriber disconnected or withOnyx wrapped component unmounted.
    if (_.every(callbackToStateMapping(mapping => mapping !== config)) {
      return;
    }

@marcaaron marcaaron Mar 7, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why is it considered a bad practice and what kinds of problems do you think it can cause? We are not accessing the connection mapping besides internally once it is passed here so I did not worry about mutations in this case.

@kidroca kidroca Mar 7, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what kinds of problems do you think it can cause?

For this particular usage primarily confusion
You know the code a great deal and that a mutation won't cause a problem in this case.
Rory knows the code as well yet it wasn't obvious mapping received connectionID through mutation

Why is it considered a bad practice

In particular for mutating input arguments:

  1. Unexpected behavior - unless the function's job is to mutate the object. You would not expect a function like print(object) to mutate the passed object, why should a function like connect be any different?
  2. You don't see the whole object shape (reading code) if some properties are added later with mutation

small airbnb thread on the subject of param reasinging: airbnb/javascript#641

In general:
It depends on the case. Contexts that allow mutations usually enforce it in an action, so it's clear it's intentional and you capture a stack trace

@marcaaron marcaaron Mar 7, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree it depends on the context/case, but not sure if I agree there is anything inherently confusing about this. I might not be understanding the full argument about why this is "bad" or what the unexpected behavior would be in this case.

Confusion is subjective. Someone could be equally confused about why we are using the spread syntax or checking for equality on an object reference (not very obvious IMO). I'm not convinced there's anything we need to do here, but do appreciate the thoughts.

@kidroca kidroca Mar 7, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Less confusing way with mutation:

    const connectionID = lastConnectionID++;
    mapping.connectionID = connectionID;
    callbackToStateMapping[connectionID] = mapping;

This also makes the mutation obvious

@marcaaron marcaaron Mar 8, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Noted and PRs Welcome 😆


if (mapping.initWithStoredValues === false) {
return connectionID;
Expand Down