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.

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

feat(observable): BREAKING return disposer instead of context for chaining #7994

Merged
merged 3 commits into from
Jun 10, 2022

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Jun 7, 2022

This PR makes observable easier to use by changing it's methods return values:

  • return disposer for on/once
  • void for fire/off

This means this PR is BREAKING.
As we discussed chainable objects are overrated and IMO are bad design because then a simple change such as this proposal becomes breaking.

So instead of:

const cb = () => {
 ...
}
obj.on('event', cb)
const dispose = () => obj.off('event', cb);
...

dispose()

This is possible

const dispose = obj.on('event', () => {
  ...
})
...

dispose()

For the dev (me for example) it saves a lot of repetitive code. It makes things easier.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2022

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.06 |    75.98 |   85.69 |   82.78 |                                               
 fabric.js |   83.06 |    75.98 |   85.69 |   82.78 | ...,30854,30928,30939-31004,31127,31226,31462 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123 ShaMan123 requested a review from asturur June 7, 2022 11:02
@fabricjs fabricjs deleted a comment from github-actions bot Jun 7, 2022
@ShaMan123 ShaMan123 requested a review from melchiar June 8, 2022 15:48
@asturur
Copy link
Member

asturur commented Jun 10, 2022

More than overrated chaining, the issue is that once you opt for chaining, you can't return anything anymore, and so you cut away a bunch of comfortable patterns. Having 10% of method chainable is not really an advantage then.
We should make clear somewhere that chaining is a leftover and developers shouldn't count on it

@ShaMan123
Copy link
Contributor Author

More than overrated chaining, the issue is that once you opt for chaining, you can't return anything anymore, and so you cut away a bunch of comfortable patterns. Having 10% of method chainable is not really an advantage then.
We should make clear somewhere that chaining is a leftover and developers shouldn't count on it

YES!!

@asturur asturur changed the title feat(observable): return values feat(observable): BREAKING return disposer instead of context for chaining Jun 10, 2022
@ShaMan123 ShaMan123 merged commit 8819ae4 into master Jun 10, 2022
@ShaMan123 ShaMan123 deleted the feat-observable-return-value branch September 11, 2022 23:40
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.

2 participants