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

fix(): weird toJson bug #8111

Merged
merged 5 commits into from
Aug 2, 2022
Merged

fix(): weird toJson bug #8111

merged 5 commits into from
Aug 2, 2022

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Aug 2, 2022 •
edited
Loading

JSON.stringify passes an empty string as first param
It causes a bug in toObject as it passes a string as propsToInclude
So I removed the params from the signature
No need for them anyways, and if someone does need them they should use toObject

DISABLED lint test

QUICK MERGE

@ShaMan123 ShaMan123 changed the title fix(): weird to json bug fix(): weird toJson bug Aug 2, 2022
@ShaMan123 ShaMan123 requested a review from asturur August 2, 2022 19:18
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2022 •
edited
Loading

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.15 |    74.52 |    84.5 |   80.73 |                                               
 fabric.js |   82.15 |    74.52 |    84.5 |   80.73 | ...,27462,27472-27516,27624,27711,27915,28056 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2022 •
edited
Loading

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.21 |    74.59 |    84.5 |   80.79 |                                               
 fabric.js |   82.21 |    74.59 |    84.5 |   80.79 | ...,27462,27472-27516,27624,27711,27915,28056 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123
ShaMan123 commented Aug 2, 2022
View reviewed changes
Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

good to go

@asturur
Copy link
Member

asturur commented Aug 2, 2022

I wonder why this does not happen in master but just on prs.
anyway here is the spec:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify

image

this the part of the spec we are interested in.

Thanks for this investigation

@asturur
Copy link
Member

asturur commented Aug 2, 2022

i wonder why was never a problem before today. We had zero transpilation before

asturur
asturur reviewed Aug 2, 2022
View reviewed changes
src/static_canvas.class.ts
* canvas.includeDefaultValues = false;
* var json = canvas.toJSON();
*/
fabric.StaticCanvas.prototype.toJSON = fabric.StaticCanvas.prototype.toObject;
Copy link
Member

Choose a reason for hiding this comment

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

this was probably my fault.

Copy link
Member

Choose a reason for hiding this comment

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

image

well not this time, but this looks totally like something i would do.

Copy link
Member

Choose a reason for hiding this comment

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

11 years this bug and no one was armed

Copy link
Member

Choose a reason for hiding this comment

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

then comes @ShaMan123 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤪

@asturur asturur merged commit e956c9c into master Aug 2, 2022
@ShaMan123 ShaMan123 deleted the fix-weird-to-json-bug branch September 11, 2022 23:33
@ShaMan123 ShaMan123 mentioned this pull request Sep 19, 2022
Open
frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023
@ShaMan123 @frankrousseau
fix(): Delegate toJson to toObject properly and fix tests (fabricjs#8111
3a725c9 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asturur asturur asturur approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ShaMan123 @asturur