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(Canvas): Safeguard from multiple initialization #7776

Merged
merged 13 commits into from
Mar 30, 2022
Merged

fix(Canvas): Safeguard from multiple initialization #7776

merged 13 commits into from
Mar 30, 2022

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Mar 8, 2022 •
edited
Loading

Will throw an error in this context:

new fabric.Canvas(el);
new fabric.Canvas(el); // will throw

@asturur why was some logic set up to enable multiple canvas initialization?
The tests that fail, fail because of it. Updated tests
I have no problem removing them and adding a multiple init test that should throw.

If there's a need to enable multiple init (I can't imagine why) I can patch up the PR accordingly

This will put an end to confusing bugs that are caused by this.

Motivation #7755 and more

dbl-init-error

@ShaMan123 ShaMan123 requested a review from asturur March 8, 2022 10:07
@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 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  |   83.16 |    76.55 |   86.38 |   82.88 |                                               
 fabric.js |   83.16 |    76.55 |   86.38 |   82.88 | ...,29865,29990,30070-30135,30258,30357,30574 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123 ShaMan123 changed the title fix(Canvas): Safeguard from double initialization fix(Canvas): Safeguard from multiple initialization Mar 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 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  |   50.47 |    41.53 |   48.11 |   50.07 |                                               
 fabric.js |   50.47 |    41.53 |   48.11 |   50.07 | ...-30412,30482,30537,30548-30551,30574-30598 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 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  |   83.17 |    76.57 |   86.38 |   82.89 |                                               
 fabric.js |   83.17 |    76.57 |   86.38 |   82.89 | ...,29865,29990,30070-30135,30258,30357,30574 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 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  |   83.16 |    76.54 |   86.38 |   82.88 |                                               
 fabric.js |   83.16 |    76.54 |   86.38 |   82.88 | ...,29865,29990,30070-30135,30258,30357,30574 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123 ShaMan123 requested a review from melchiar March 9, 2022 07:27
@ShaMan123 ShaMan123 mentioned this pull request Mar 18, 2022
Closed
@asturur
Copy link
Member

asturur commented Mar 25, 2022

How do you find yourself initializing more than once?
I did it in the past but more as an escape hatch and is not defintely a feature.

@ShaMan123
Copy link
Contributor Author

I don't. I see it happening in strange bug reports. If you recall one you looked into and I closed.
Someone trying to init canvas from some event handler that runs more than once.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 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  |   83.16 |    76.56 |   86.29 |   82.88 |                                               
 fabric.js |   83.16 |    76.56 |   86.29 |   82.88 | ...,29912,30037,30117-30182,30305,30404,30640 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 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  |   83.17 |    76.56 |   86.29 |   82.89 |                                               
 fabric.js |   83.17 |    76.56 |   86.29 |   82.89 | ...,29915,30040,30120-30185,30308,30407,30643 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 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  |   83.18 |    76.56 |   86.35 |    82.9 |                                               
 fabric.js |   83.18 |    76.56 |   86.35 |    82.9 | ...,29907,30032,30112-30177,30300,30399,30635 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 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  |   83.23 |     76.6 |   86.35 |   82.95 |                                               
 fabric.js |   83.23 |     76.6 |   86.35 |   82.95 | ...,29907,30032,30112-30177,30300,30399,30635 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 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  |   83.18 |    76.56 |   86.35 |    82.9 |                                               
 fabric.js |   83.18 |    76.56 |   86.35 |    82.9 | ...,29907,30032,30112-30177,30300,30399,30635 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123
ShaMan123 commented Mar 29, 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.

Added the DEV_MODE comment
Ready to merge

@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 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  |   83.23 |     76.6 |   86.35 |   82.95 |                                               
 fabric.js |   83.23 |     76.6 |   86.35 |   82.95 | ...,29909,30034,30114-30179,30302,30401,30637 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 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  |   83.18 |    76.56 |   86.35 |    82.9 |                                               
 fabric.js |   83.18 |    76.56 |   86.35 |    82.9 | ...,29909,30034,30114-30179,30302,30401,30637 
-----------|---------|----------|---------|---------|-----------------------------------------------

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 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  |   83.18 |    76.56 |   86.35 |    82.9 |                                               
 fabric.js |   83.18 |    76.56 |   86.35 |    82.9 | ...,29909,30034,30114-30179,30302,30401,30637 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123 ShaMan123 merged commit ec8d379 into master Mar 30, 2022
@ShaMan123 ShaMan123 deleted the safeguard-dbl-init branch March 30, 2022 16:17
@ShaMan123 ShaMan123 mentioned this pull request Sep 20, 2022
Open
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

@melchiar melchiar Awaiting requested review from melchiar

Assignees
No one assigned
Labels
fix-next
Projects
None yet
Milestone
No milestone
Development

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