handle undefined in shoebox

[kellyselden]

Closes #137

[kellyselden]

After talking to @tomdale, I changed the implementation. After verifying that shoebox.retrieve returns undefined if the key doesn't exist in ember-fastboot/ember-cli-fastboot#406, it doesn't make much sense to jam undefined in the shoebox at all. So this now ignores undefined instead.

[kratiahuja]

We should also log that this key won't be present.

[stefanpenner]

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.

[kratiahuja]

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.

[stefanpenner]

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.

[kellyselden]

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.

[kratiahuja]

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.

[stefanpenner]

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)

[kratiahuja]

I am +1 on error which would even fail the visit request I believe.

[kellyselden]

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.

[kratiahuja]

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.