Skip to content

Fix JSON.stringify causing TypeErrors#2029

Merged
ricardograca merged 4 commits intobookshelf:masterfrom
anajavi:fix-2028
Oct 5, 2019
Merged

Fix JSON.stringify causing TypeErrors#2029
ricardograca merged 4 commits intobookshelf:masterfrom
anajavi:fix-2028

Conversation

@anajavi
Copy link
Contributor

@anajavi anajavi commented Oct 4, 2019

Introduction

Fix JSON.stringify throwing TypeError if Model object is placed as object property or in an array.

Motivation

JSON.stringify calls .toJSON on object to be serialized. When called in array, it passes index as parameter to .toJSON. When called in object it passes the property name as parameter:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#toJSON_behavior

Bookshelf model accepts options object as parameter for toJSON, it however does not check if passed parameter is object before trying to put properties in it. This results in TypeError: Cannot create property 'visibility' on string 'propertyname'

Proposed solution

The proposed solution is to check if options is really an object by checking it with lodash.isPlainObject.

Current PR Issues

Not benchmarked this, so can't know the performance implications.

Alternatives considered

Simpler Object check might be more performant. One from redux:
https://github.com/reduxjs/redux/blob/master/src/utils/isPlainObject.ts

@ricardograca ricardograca self-requested a review October 5, 2019 15:40
Copy link
Member

@ricardograca ricardograca left a comment

Choose a reason for hiding this comment

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

Looks great, but the simpler object check would be preferable.

@anajavi
Copy link
Contributor Author

anajavi commented Oct 5, 2019

I hope you can release this fix soon, as it is blocking upgrade to 1.0 for me.

Copy link
Member

@ricardograca ricardograca left a comment

Choose a reason for hiding this comment

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

Looks good.

@ricardograca ricardograca merged commit e87b9e4 into bookshelf:master Oct 5, 2019
@ricardograca
Copy link
Member

This will be released very soon as there's no reason not to.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants