Skip to content

Conversation

@acupoftee
Copy link

This PR addresses Issues #10 and #18

My approach for this iteration was to add a separate check for empty objects and arrays in background.js before the text is passed to the tokenizer:

    // If it's an empty object or array, display without the formatting
    else if (Object.entries(obj).length === 0 || obj.length === 0) {
      port.postMessage(['NOT JSON', 'empty objects']);
      port.disconnect();
      return;
    }

This will return the empty object or array without the styling to prevent formatting bug.

@acupoftee
Copy link
Author

I meant to make a feature branch before hand for this PR. This one at least has fixes for #19 as well as the empty object check.

return;
}
// If there's an empty object or array, return JSON as is
else if (Object.entries(obj).length === 0 || obj.length === 0) {
Copy link

Choose a reason for hiding this comment

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

Wouldn't obj.length === 0 be sufficient?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. obj.length === 0 would only work for the array since arrays have a length property, but not objects. (This can also be verified by trying object.hasOwnProperty('length');

Since [Object.entries()](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries) returns an array of key value pairs, we can use that to see if an object has any content. (you could do this with .keys() too. I tend to use .entries() for semantics).

@pedzed
Copy link

pedzed commented Sep 29, 2019

Looks good to me 👍

@nicole-ashley
Copy link
Owner

Thanks for the PR! I admit I missed the GitHub notifications coming up on this repo, or perhaps they're not configured correctly. I'll check and try to stay on top of this in the future.

@acupoftee, how does this manifest in the browser when there's an empty object or array? Could you include a screenshot for ease of review? From a code point of view I'm happy with the change, I just want to check how it displays.

@acupoftee
Copy link
Author

No problem! Will do, here they are below:

Empty object:
Screen Shot 2019-09-30 at 7 02 03 PM

Empty array:
Screen Shot 2019-09-30 at 7 02 24 PM

@nicole-ashley
Copy link
Owner

Thanks! In the future I may loop back to make it so that the formatter still instantiates in this situation (without hanging) but this is much better than a hang.

I'll look at releasing a new version shortly.

@nicole-ashley nicole-ashley merged commit e6fd5fd into nicole-ashley:master Sep 30, 2019
@acupoftee
Copy link
Author

Sounds awesome. Thank you for reviewing and for your work on this version!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants