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

Remove Events System #37

Closed
ajoslin opened this issue Jan 16, 2014 · 14 comments
Closed

Remove Events System #37

ajoslin opened this issue Jan 16, 2014 · 14 comments

Comments

@ajoslin
Copy link
Owner

ajoslin commented Jan 16, 2014

It seems to me the events system is a useless bloat feature: I added it at the start just 'as a thing' (I mean the tracker.on('done', cb) kind of thing).

  1. Takes up more KB, bad for mobile
  2. Almost everything it can do is done well with promise events or a tiny bit more work for the developer
  3. Does anyone actually use the events?

If we remove it, promise-tracker can become a good micro-library that does only what you need.

@ajoslin ajoslin closed this as completed Jan 22, 2014
@tlvince
Copy link

tlvince commented Jan 23, 2014

@ajoslin, decided against it? Seems worth doing to me.

@ajoslin ajoslin reopened this Jan 23, 2014
@ajoslin
Copy link
Owner Author

ajoslin commented Jan 23, 2014

Woohoo, feedback! Thanks @tlvince.

I will probably do it within the next few weeks, when I use this on a new project that requires minimum-minimum filesize.

@jtammen
Copy link

jtammen commented Jan 24, 2014

Hey @ajoslin,

even though I am not currently using the events, I was thinking about using them in my current project. I need to achieve the following:

Let's say I have a data entry form with several input fields. Each field's data gets saved separately on blur by calling some server API. Now I'd like to show an activity indicator next to each field when data is being saved. I have only one service method that is responsible for saving the data. My idea was to use one single promise tracker there that will be given the current form attribute name in the eventData argument.
Then I would attach a directive (e.g. busy-when="trackerName" busy-when-attribute="attrName") to the form input fields which would listen to the start and done events of the given promise tracker, check via the given eventData whether the event is targeted to the actual form input and then for example add/remove some CSS classes to/from the element.

Do you think sth like this would be still doable without those event hooks? Or maybe there is an even simpler solution ... maybe I could just use one (dynamically named) promise tracker for each individual form input.

I would really appreciating your feedback on this! Thanks.

@ajoslin
Copy link
Owner Author

ajoslin commented Jan 24, 2014

I think one promiseTracker per input would be the simplest, (there is very little overhead per promiseTracker).

You could do it this way, if I added register and deregister methods, register for more explicit registration and deregister for memory management.

$scope.model = {
  foo: 'foo',
  bar: 'BAR',
  baz: 'bazzy'
};
var trackers = {};
angular.forEach($scope.model, function(value, key) {
  trackers[key] = promiseTracker.register(key, {/*options*/});
});

$scope.submit = function(key) {
  trackers[key].addPromise(SubmitService.submit(key, $scope.model[key]));
};

$scope.$on('$destroy', function() {
  angular.forEach(trackers, function(value, key) {
    promiseTracker.deregister(key);
  });
});

@jtammen
Copy link

jtammen commented Jan 25, 2014

Thanks for the feedback, that sounds plausible!

@ajoslin
Copy link
Owner Author

ajoslin commented Jan 26, 2014

Will add a few more tests and push today: register and deregister functions, and no events system.

The old version will still be available for people who don't want to migrate their code.

ajoslin added a commit that referenced this issue Jan 27, 2014
BREAKING CHANGE:

The events system has been removed.  That means you can no longer do
`.on()` and `.off()`.  In essence, events were a poor way of doing
'hooks' on promises.  See [this
issue](#37) for
more information.

[Open an
issue of your own](https://github.com/ajoslin/angular-promise-tracker/issues) if you
were using events and need help migrating to use anotehr method.

Check [the
README](https://github.com/ajoslin/angular-promise-tracker/tree/master/README.md)
for details on the current API for promiseTracker.
@ajoslin
Copy link
Owner Author

ajoslin commented Jan 27, 2014

Landed in v2.0.0-beta1. See the changelog. https://github.com/ajoslin/angular-promise-tracker/blob/master/CHANGELOG.md

@ajoslin ajoslin closed this as completed Jan 27, 2014
@ajoslin
Copy link
Owner Author

ajoslin commented Jan 27, 2014

Also, filesize shaved by almost 40% with the new change. :-)

@ajoslin
Copy link
Owner Author

ajoslin commented Jan 27, 2014

Bumped to v2.0.0-beta2 with a fix. every time!

  • Release
  • Find problem
  • Rerelease

Also going to reopen this issue for problems.

@ajoslin ajoslin reopened this Jan 27, 2014
@tlvince
Copy link

tlvince commented Jan 27, 2014

👍

@ajoslin
Copy link
Owner Author

ajoslin commented Jan 28, 2014

We could break things even more (and simplify them) before v2.0.0.

We could remove the whole id system altogether.

var myTracker = new promiseTracker(options); or var tracker = promiseTracker(options).

The main downside is this requires people to create a service to store their trackers. Or store them on rootScope. Both of which I usually do already.

This means we could get rid of the whole weird register and deregister system. We could still have a destroy() function for manual tracker cleanup.

Thoughts @tlvince @jtammen?

@ajoslin
Copy link
Owner Author

ajoslin commented Jan 28, 2014

Also we could remove the $http tracker sugar option, or possibly make it an optional addition of some sort.

It's nice because it can save you a tiny bit of code.. but the downside is it makes people usually put tracker: keys into their services' $http calls. The way it should be done is to put the tracker on the promise that does everything that you are tracking, usually on the controller side.

$scope.register = function(user, pass) {
  var loginPromise = AuthService.register(user, pass).then(function() {
    return AuthService.login(user, pass);
  }).then(function() {
    return doSomethingElse();
  }).then(function() {
    alert('login done!');
  });
  $scope.tracker.addPromise(loginPromise);
};

@jtammen
Copy link

jtammen commented Jan 28, 2014

@ajoslin The last example you posted looks almost like the way I am already doing it – so removing the ID stuff and the $http sugar would be OK for me, personally.

But if I understand your proposal correctly, then this would break for example angular-busy, which relies on being able to identify a certain tracker by its ID, wouldn't it?

@ajoslin
Copy link
Owner Author

ajoslin commented Jan 28, 2014

ok I will leave it for now since it does have uses!

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

No branches or pull requests

3 participants