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.

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

Jump to bottom

handle undefined in shoebox #138

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

handle undefined in shoebox #138

Show file tree
Hide file tree
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
2 changes: 2 additions & 0 deletions src/ember-app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,8 @@ function createShoebox(doc, fastbootInfo) {

let value = shoebox[key];
let textValue = JSON.stringify(value);
if (textValue === undefined) { continue; }
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@stefanpenner stefanpenner Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@kellyselden kellyselden Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


textValue = escapeJSONString(textValue);

let scriptText = doc.createRawHTMLSection(textValue);
Expand Down
9 changes: 9 additions & 0 deletions test/fastboot-shoebox-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ describe("FastBootShoebox", function() {
expect(html).to.include('<script type="fastboot/shoebox" id="shoebox-key4">{"nastyScriptCase":"\\u003cscript\\u003ealert(\'owned\');\\u003c/script\\u003e\\u003c/script\\u003e\\u003c/script\\u003e"}</script>');

expect(html).to.include('<script type="fastboot/shoebox" id="shoebox-key5">{"otherUnicodeChars":"\\u0026\\u0026\\u003e\\u003e\\u003c\\u003c\\u2028\\u2028\\u2029\\u2029"}</script>');

// ignores undefined
expect(html).to.not.include('shoebox-key6');
});
});

Expand All @@ -43,6 +46,9 @@ describe("FastBootShoebox", function() {
expect(html).to.include('<script type="fastboot/shoebox" id="shoebox-key4">{"nastyScriptCase":"\\u003cscript\\u003ealert(\'owned\');\\u003c/script\\u003e\\u003c/script\\u003e\\u003c/script\\u003e"}</script>');

expect(html).to.include('<script type="fastboot/shoebox" id="shoebox-key5">{"otherUnicodeChars":"\\u0026\\u0026\\u003e\\u003e\\u003c\\u003c\\u2028\\u2028\\u2029\\u2029"}</script>');

// ignores undefined
expect(html).to.not.include('shoebox-key6');
});
});

Expand All @@ -63,6 +69,9 @@ describe("FastBootShoebox", function() {
expect(html).to.not.include('<script type="fastboot/shoebox" id="shoebox-key4">{"nastyScriptCase":"\\u003cscript\\u003ealert(\'owned\');\\u003c/script\\u003e\\u003c/script\\u003e\\u003c/script\\u003e"}</script>');

expect(html).to.not.include('<script type="fastboot/shoebox" id="shoebox-key5">{"otherUnicodeChars":"\\u0026\\u0026\\u003e\\u003e\\u003c\\u003c\\u2028\\u2028\\u2029\\u2029"}</script>');

// ignores undefined
expect(html).to.not.include('shoebox-key6');
});
});
});
1 change: 1 addition & 0 deletions test/fixtures/shoebox/fastboot/fastboot-test.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.