feat(): support aborting promises/requests #7827

ShaMan123 · 2022-03-24T08:45:32Z

Motivation #7075

Gist
https://developer.mozilla.org/en-US/docs/Web/API/AbortController
https://dev.to/abdus/terminating-dom-operations-at-will-abortcontroller-in-javascript-1icm
https://nodejs.org/api/globals.html#class-abortcontroller

In Action
https://codesandbox.io/s/mpcq6z

clear canvas right before reload instead of before enlivening, which may take time in case of heavy images/resources

https://nodejs.org/api/globals.html#class-abortcontroller

ShaMan123 · 2022-03-24T09:35:57Z

I think we should consider migrating XMLHttpRequest to fetch

ShaMan123 · 2022-03-24T09:36:31Z

src/mixins/canvas_serialization.mixin.js

    if (!json) {
-      return;
+      return Promise.reject(new Error('fabric.js: `json` is undefined'));


asturur · 2022-06-26T14:22:16Z

read all the PR, and the only thing i disagree is providing the abort controller ourselves.
The abort signal from the controller should be sent from outside from the developer ( that wants to use it ), this is how i interacted with aws uploads in s3 for example, they accept an abort signal from outside, but don't create one for you.
This would also remove the issue with the supported node version.

We would also not need to provide a method to interrup the current task, supporting the existance of an abort signal is all we should surface.

ShaMan123

the only thing i disagree is providing the abort controller ourselves.

src/mixins/canvas_serialization.mixin.js

src/shapes/image.class.js

src/mixins/canvas_serialization.mixin.js

asturur

I insist for the Abort controller being passed from outside in the functions where we decided to generate it automatically.
That does not change the functionality you are proposing, and allows for better integration of loading tasks.
The operation can be cancelled from inside or from outside, and most importantly the same abort signal from the used abort controller can be passed to other systems that needs to be cancelled on coordination.

Other than it allow us to leave node 14 support on.

ShaMan123 · 2022-06-28T08:35:18Z

I still don't understand how it solves the bug I described or how I can set src using img.set. I am unconvinced.
And regarding passing the signal down - it's how I wrote it - it does that exactly.

asturur · 2022-06-28T08:40:59Z

i don't see set('src', value) anywhere defaulting to setSrc, where that happens?

ShaMan123 · 2022-06-28T08:44:20Z

i don't think we should guard against this internally. I want to leave loading patterns to the developer.

I can understand this and live with it but I think it's a bad idea because it will surface fake bugs from devs. And it is expected behavior not to have race conditions.

But not img.set('src',...)

EDITED

I gave a deeper look. Does img.set('src',...) do anything? Seems not. If that's the case I don't have any objections and would propose to support img.set('src',...) in the future.

ShaMan123 · 2022-06-28T08:49:51Z

img.set('src',...)

It doesn't which IMO should be viewed as a bug

asturur · 2022-06-28T08:50:18Z

indeed src is not an image of the property, setSrc is like, a, change the element. It does not even update it, it totally changes it ( and that is fine because the previous one could be a canvas element ).
I m not even sure if setSrc is a good idea in general, it looks like a wrapper for setElement with a loader, but on top of that then changes the image size, while setElement does't.
No one of them is resetting cropX and cropY, creating potentially weird visual bugs when cropX is bigger than the new width and similar for cropY.

ShaMan123 · 2022-06-28T08:52:00Z

Good point
So perhaps src should only exist on from/to object

I now understand your point

asturur · 2022-06-28T08:59:24Z

img.set('src',...)

It doesn't which IMO should be viewed as a bug

probably the bug is the name of the method. Probably that is setElementFromSrc
fabric.Image does not have a src, it has a drawable element, we have utils to load new drawable elements ( like fabric.util.loadImage ) and we can improve how they are swapped the element.

ShaMan123 · 2022-06-28T09:20:16Z

Yeah we should rename and add logic
I think we should rename Image as well because it can be Video as well

github-actions · 2022-06-28T10:17:32Z

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.14 |   85.68 |   82.91 |                                               
 fabric.js |   83.18 |    76.14 |   85.68 |   82.91 | ...,30945,31019,31030-31095,31218,31317,31553 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-06-28T10:21:34Z

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.22 |    76.14 |   85.68 |   82.95 |                                               
 fabric.js |   83.22 |    76.14 |   85.68 |   82.95 | ...,30949,31023,31034-31099,31222,31321,31557 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-06-28T10:26:11Z

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.14 |   85.68 |   82.91 |                                               
 fabric.js |   83.18 |    76.14 |   85.68 |   82.91 | ...,30949,31023,31034-31099,31222,31321,31557 
-----------|---------|----------|---------|---------|-----------------------------------------------

github-actions · 2022-06-28T10:28:22Z

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.14 |   85.68 |   82.91 |                                               
 fabric.js |   83.18 |    76.14 |   85.68 |   82.91 | ...,30949,31023,31034-31099,31222,31321,31557 
-----------|---------|----------|---------|---------|-----------------------------------------------

ShaMan123

READY!

asturur · 2022-06-30T14:11:08Z

Merging this, i ll follow up restoring node 14 adding a polyfill in the dev-deps

ShaMan123 added 22 commits March 23, 2022 09:10

fix(loadFromJSON): clear canvas

735e854

clear canvas right before reload instead of before enlivening, which may take time in case of heavy images/resources

Update canvas_serialization.mixin.js

cb13c0c

feat(util): support aborting loadImage

d927c7e

refactor renderOnAddRemove

47e98f4

Update misc.js

172d550

parallel promise

65e6984

live namespace

b8f0ada

enliven options

c02b917

Update misc.js

c1fdfd4

Update image.class.js

cc0539d

Update pattern.class.js

3099a65

aborting

f46d833

aborting svg parsing

54b01e5

bump node to support AbortController

5857859

https://nodejs.org/api/globals.html#class-abortcontroller

listen to abort once

4ffefd6

bump node tests

aaf3f5b

lint + JSDOC

da720f5

Update parser.js

82c4e9e

feat(Image): abort setSrc

b1470dd

Update .eslintrc.json

20e8e87

Update canvas_serialization.mixin.js

ea049c9

remove resolveNamespace

92f5336

ShaMan123 commented Mar 24, 2022

View reviewed changes

ShaMan123 added 6 commits March 24, 2022 11:41

Update pattern.class.js

7510ab1

Update misc.js

0565006

consolidate options object

88b3f0c

abort image filter loading

3e0f7b6

fire loading:aborted event

9f82c17

Update object.class.js

bb26215

ShaMan123 commented Jun 26, 2022

View reviewed changes

src/mixins/canvas_serialization.mixin.js Outdated Show resolved Hide resolved

src/shapes/image.class.js Outdated Show resolved Hide resolved

asturur reviewed Jun 28, 2022

View reviewed changes

src/mixins/canvas_serialization.mixin.js Show resolved Hide resolved

asturur requested changes Jun 28, 2022

View reviewed changes

ShaMan123 added 5 commits June 28, 2022 12:45

incoming signal **ONLY** + revert node to v14

4f5b6e2

missed out

52ba6fb

tests

ec86828

Update canvas_serialization.mixin.js

a61e3e6

conform rejection object

b15bea1

Update canvas_serialization.mixin.js

db16268

ShaMan123 added 2 commits June 28, 2022 13:23

restore config for tests

efaf0b4

revert node v

06a1d62

ShaMan123 requested a review from asturur June 28, 2022 10:33

ShaMan123 commented Jun 28, 2022

View reviewed changes

asturur approved these changes Jun 30, 2022

View reviewed changes

asturur merged commit 78b7029 into master Jun 30, 2022

ShaMan123 deleted the fix-json-load branch September 11, 2022 23:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(): support aborting promises/requests #7827

feat(): support aborting promises/requests #7827

ShaMan123 commented Mar 24, 2022 •

edited

ShaMan123 commented Mar 24, 2022

ShaMan123 Mar 24, 2022

asturur commented Jun 26, 2022

ShaMan123 left a comment

asturur left a comment •

edited

ShaMan123 commented Jun 28, 2022

asturur commented Jun 28, 2022

ShaMan123 commented Jun 28, 2022 •

edited

ShaMan123 commented Jun 28, 2022

asturur commented Jun 28, 2022

ShaMan123 commented Jun 28, 2022

asturur commented Jun 28, 2022

ShaMan123 commented Jun 28, 2022

github-actions bot commented Jun 28, 2022 •

edited

github-actions bot commented Jun 28, 2022 •

edited

github-actions bot commented Jun 28, 2022 •

edited

github-actions bot commented Jun 28, 2022 •

edited

ShaMan123 left a comment

asturur commented Jun 30, 2022

feat(): support aborting promises/requests #7827

feat(): support aborting promises/requests #7827

Conversation

ShaMan123 commented Mar 24, 2022 • edited

ShaMan123 commented Mar 24, 2022

ShaMan123 Mar 24, 2022

Choose a reason for hiding this comment

asturur commented Jun 26, 2022

ShaMan123 left a comment

Choose a reason for hiding this comment

asturur left a comment • edited

Choose a reason for hiding this comment

ShaMan123 commented Jun 28, 2022

asturur commented Jun 28, 2022

ShaMan123 commented Jun 28, 2022 • edited

ShaMan123 commented Jun 28, 2022

asturur commented Jun 28, 2022

ShaMan123 commented Jun 28, 2022

asturur commented Jun 28, 2022

ShaMan123 commented Jun 28, 2022

github-actions bot commented Jun 28, 2022 • edited

Code Coverage Summary

github-actions bot commented Jun 28, 2022 • edited

Code Coverage Summary

github-actions bot commented Jun 28, 2022 • edited

Code Coverage Summary

github-actions bot commented Jun 28, 2022 • edited

Code Coverage Summary

ShaMan123 left a comment

Choose a reason for hiding this comment

asturur commented Jun 30, 2022

ShaMan123 commented Mar 24, 2022 •

edited

asturur left a comment •

edited

ShaMan123 commented Jun 28, 2022 •

edited

github-actions bot commented Jun 28, 2022 •

edited

github-actions bot commented Jun 28, 2022 •

edited

github-actions bot commented Jun 28, 2022 •

edited

github-actions bot commented Jun 28, 2022 •

edited