handle undefined in shoebox #138
Conversation
kellyselden
force-pushed
the
shoebox-undefined
branch
from
5fafef8
to
2939f47
Compare
|
After talking to @tomdale, I changed the implementation. After verifying that |
| @@ -450,6 +450,8 @@ function createShoebox(doc, fastbootInfo) { | |||
|
|
|||
| let value = shoebox[key]; | |||
| let textValue = JSON.stringify(value); | |||
| if (textValue === undefined) { continue; } | |||
There was a problem hiding this comment.
We should also log that this key won't be present.
There was a problem hiding this comment.
Im not sure always logging makes sense. Seems like it could be valid to be undefined?
If it is exceptional then sure, otherwise at most a development only log, but I'm inclined to not over-log.
There was a problem hiding this comment.
I don't think it is valid usecase to have it undefined. It's more exceptional case IMO. Since we are magically dropping the key and value from shoebox, it might be a surprise. I was leaning towards a debug log.
There was a problem hiding this comment.
If its exceptional, why wouldn't we error entirely (but with a good error)? This must be an "Expected" workflow right? Or what am I missing.
There was a problem hiding this comment.
Expected workflow is I'm jamming a bunch of keys and values in the shoebox "blindly" on one side, then retrieving them on the other. I expect the values to be the exact same on the other side, whether undefined or not.
The alternative to this PR is to check against undefined every time I put something into the shoebox. This PR seems more developer friendly.
There was a problem hiding this comment.
I don't think the purpose of shoebox is to have empty members. It can have missing members if the app choses to not put something in the shoebox. It also so happens that retrieve returns undefined if key is not present so that the app can make over the wire call if it wishes.
IMO the app is responsible for deciding what all he wants int he shoebox so either it should put correct values and we error if it is undefined. If we are magically dropping something that the app expected to be in the shoebox, we should log. That's the only reason why I proposed for logging it since app is expecting it in the shoebox.
There was a problem hiding this comment.
That's the only reason why I proposed for logging it since app is expecting it in the shoebox.
I'm curious why we wouldn't error? (if it is expected)
There was a problem hiding this comment.
I am +1 on error which would even fail the visit request I believe.
There was a problem hiding this comment.
I don't see the need to error. Why not let people put in and take out undefined? Whether or not we actually serialize the key and value should be a private implementation detail.
There was a problem hiding this comment.
Because undefined is not a valid value from the shoebox perspective. The user is most likely going to assume he gets valid key, value pairs from the shoebox always. It's more of a data store. If the API failed for some reason during server side rendering they need to be aware of it as well. Erroring it out makes them aware of such a state.
kellyselden
force-pushed
the
shoebox-undefined
branch
from
2939f47
to
9a4aa1c
Compare
Closes #137