Add event.namespace support #15

Merged
merged 1 commit into from Mar 30, 2016

Projects

None yet

2 participants

@Ke-
Contributor
Ke- commented Mar 25, 2016

One of the limitations of (riot) observable is the lack of name spacing.

This is particularly apparent when attempting to bind multiple tags to a single datastore. For example:

// <tag1>
this.on('mount',function(){
    usersStore.on('changed.tag1', function(items){ 
        this.update() 
    }) 
})
this.on('unmount',function(){
    usersStore.off('changed.tag1')
})

// <tag2>
this.on('mount',function(){
    usersStore.on('changed.tag2', function(items){ 
        this.update() 
    }) 
})
this.on('unmount',function(){
    usersStore.off('changed.tag2')
})

With the above example ideally we would want both tags to update when the store emits a "changed" event —currently it requires the store to emit both events: 'changed.tag1' and 'changed.tag2'.

With namespacing both tags will update when a 'changed' event is triggered. Equally important on unmount only the change event related to the tag in question will be removed. All 'changed' events can still be removed by calling .off('changed').

This minor change massively increases the power and flexibility of the emitter with minimal downside. Yes, it's possible that a few current users may see a change in behaviour if they use dots in their events, but I suspect this user count will be low as the dot is typically used to address namespacing (see jQuery as an example: https://api.jquery.com/event.namespace/ ).

This is one of the reasons why I don't use Riots observable. It would be great if it had this feature.

Thanks.

Another example:

var test = {}
observable(test)

test.on('hello',function(a){console.log('hello',a)}) 
test.on('hello.world',function(a){console.log('hello,world',a)}) 

test.trigger('hello','both')
test.trigger('hello.world','one')
test.off('hello.world')
test.trigger('hello','just "hello" ')
@GianlucaGuarini
Member

@Ke- thank you its looks a great feature and it does not add too much code. I would prefer to have a single commit adding this feature and also I would like to test it in a benchmark to check whether the obsevable methods will be slower. Can you squash your changes and make a new pull request please?

@Ke-
Contributor
Ke- commented Mar 25, 2016

Great; have squashed the pull request into a single commit.

Within the onEachEvent function using:

      name = name.split('.')
      ns   = name[1]
      name = name[0]

Is marginally faster; however it means events would only support a single dot.

It does however impose enforcement of sensible logic — so it may be the better of the two approaches.

@Ke-
Contributor
Ke- commented Mar 25, 2016

Changed the behaviour to enforce a single name space. So with the below:

event.name.space

The namespace will be considered name.space — this is slightly slower, but ensures a more consistent behaviour with the current release.

@Ke-
Contributor
Ke- commented Mar 29, 2016

Improved performance as per: http://jsperf.com/namespacetest2

@GianlucaGuarini
Member

could you update the doc as well please?

@Ke-
Contributor
Ke- commented Mar 29, 2016

Sure; will do.

@Ke-
Contributor
Ke- commented Mar 29, 2016

Documentation also updated.

@Ke- @Ke- Ke- Add event.namespace support
One of the limitations of (Riot) observable is the lack of name spacing.

This is particular apparent when attempting to bind multiple mounts to a single datastore. For example:

// <tag1>
this.on('mount',function(){
	usersStore.on('changed.tag1', function(items){
		this.update()
	})
})
this.on('unmount',function(){
	usersStore.off('changed.tag1')
})

// <tag2>
this.on('mount',function(){
	usersStore.on('changed.tag2', function(items){
		this.update()
	})
})
this.on('unmount',function(){
	usersStore.off('changed.tag2')
})

With the above example ideally we would want both tags to update when the store emits a "changed" event —currently it requires the store to emit both events: 'changed.tag1' and 'changed.tag2'.

With namespacing both tags will update when a 'changed' event is triggered. Equally important on unmount only the change event related to the tag in question will be removed. All 'changed' events can still be removed by calling .off('changed').

This minor change massively increases the power and flexibility of the emitter with minimal downside. Yes, it's possible that a few current users may see a change in behaviour if they use dots in their events, but I suspect this user count will be low as the dot is typically used to address namespacing (see jQuery as an example: https://api.jquery.com/event.namespace/ ).

This is one of the reasons why I don't use Riots observable — it would be great if it had this feature.

Thanks.

Another example:

var test = {}
observable(test)

test.on('hello',function(a){console.log('hello',a)})
test.on('hello.world',function(a){console.log('hello,world',a)})

test.trigger('hello','both')
test.trigger('hello.world','one')
test.off('hello.world')
test.trigger('hello','just "hello" ')

Update index.js

Corrected issue with .one

Cleaner implementation

Cleared irrelevant vars

Formatting

Formatting

Formatting

Account for multiple dots.

Formatting

Changed to enforce single namespace

More consistent behaviour.

Update index.js

Update index.js

Added documentation for namespaces

Reverted as JSperf numbers seemed off

Reverted namespace.
db47931
@GianlucaGuarini
Member

great thank you!

@GianlucaGuarini GianlucaGuarini merged commit 711457b into riot:master Mar 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@GianlucaGuarini
Member

It seems that this pull request slows down the riot observable. According to the benchmark added to the repo comparing the new observable against the old one, it seems that now it's 2x slower

new-observable#simple event x 762,496 ops/sec ±1.00% (90 runs sampled)
new-observable#namespaced event x 759,938 ops/sec ±1.11% (92 runs sampled)
new-observable#multiple events x 190,263 ops/sec ±1.78% (86 runs sampled)
old-observable#simple event x 1,372,204 ops/sec ±0.94% (90 runs sampled)
old-observable#multiple events x 244,157 ops/sec ±2.06% (85 runs sampled)
Fastest is old-observable#simple event

Maybe we need to handle this feature only using regex

@Ke-
Contributor
Ke- commented Apr 2, 2016

Yes, I would expect it to be slower due to the intermediate function. Unfortunately using a more advance regex is slower again —see below. For the absolute best performance we should consider dropping the regexp completely:

splice-observable#simple event x 1,377,984 ops/sec ±1.29% (83 runs sampled)
split-observable#namespaced event x 1,835,461 ops/sec ±0.97% (80 runs sampled)
ns-regexp-observable#namespaced event x 1,280,703 ops/sec ±1.21% (84 runs sampled)
for-observable#namespaced event x 3,815,956 ops/sec ±0.98% (81 runs sampled)
for-observable#multiple events x 1,586,047 ops/sec ±1.06% (85 runs sampled)
old-observable#namespaced event x 2,877,195 ops/sec ±0.82% (91 runs sampled)
old-observable#multiple events x 1,539,768 ops/sec ±1.28% (83 runs sampled)
Fastest is for-observable#namespaced event

Benchmarks and methods here:

var Benchmark = require('benchmark');

var onEachEventOld = function(e, fn) { e.replace(/\S+/g, fn) }

var onEachEventNSReg = function(e, fn) { e.replace(/(\w+)\.?(\S+)/g, fn) }

var onEachEventSplice = function(e, fn) { e.replace(/\S+/g, function(name, pos, ns) {
  name = name.split('.')
  ns   = name.splice(1).join('.')
  name = name[0]
  fn(name, pos, ns)  
})}

var onEachEventSplit = function(e, fn) { e.replace(/\S+/g, function(name, pos, ns) {
  name = name.split('.')
  ns   = name[1]
  name = name[0]
  fn(name, pos, ns)
})}

var onEachEventFor = function(e, fn) {
  var es = e.split(' '), l = es.length, i = 0, name, ns
  for(; i < l; i++){
    name = es[i].split('.')
    ns   = name[1]
    name = name[0]
    fn(name, i, ns)
  }
}

var suite = new Benchmark.Suite;

suite.add('splice-observable#simple event', function(){
  onEachEventSplice('click', function(){})
})

suite.add('split-observable#namespaced event', function(){
  onEachEventSplit('click.namespace', function(){})
})

suite.add('ns-regexp-observable#namespaced event', function(){
  onEachEventNSReg('click.namespace', function(){})
})

suite.add('for-observable#namespaced event', function(){
  onEachEventFor('click.namespace', function(){})
})

suite.add('for-observable#multiple events', function(){
  onEachEventFor('click click.namespace keyup', function(){})
})

suite.add('old-observable#namespaced event', function(){
  onEachEventOld('click.namespace', function(){})
})

suite.add('old-observable#multiple events', function(){
  onEachEventOld('click click.namespace keyup', function(){})
})

.on('cycle', function(event) {
  console.log(String(event.target));
})
.on('complete', function() {
  console.log('Fastest is ' + this.filter('fastest').map('name'));
})
.run({ 'async': true });
@Ke-
Contributor
Ke- commented Apr 2, 2016

Another alternative which retains compatibility with multiple dots:

var onEachEventForSlice = function(e, fn) {
  var es = e.split(' '), l = es.length, i = 0, name, indx
  for(; i < l; i++){
    name = es[i]
    indx = name.indexOf('.')
    fn(name.substring(indx), i, ~indx ? name.slice(indx + 1) : undefined)
  }
}

// for-slice-observable#namespaced event x 3,551,852 ops/sec ±0.89% (90 runs sampled)
@GianlucaGuarini
Member

The last possibility seems to be the best, I will update the code asap and release riot-observable@2.4.0

@Ke-
Contributor
Ke- commented Apr 5, 2016

Any reason why you decided to keep the regexp vs split + for approach?

Similar question applies splice vs slice — the for + slice approach appears much faster in my testing:

3,551,852 ops/sec (for-slice-observable#namespaced)
vs
1,377,984 ops/sec

@GianlucaGuarini
Member

I didn't update the script yet. Could you make another pull request please? I will be able to work on it again during the weekend

@Ke-
Contributor
Ke- commented Apr 6, 2016

Sure; will do so tomorrow.

@a-moses a-moses referenced this pull request in riot/riot May 14, 2016
Closed

reduce or completely dropping the regexp ? #1791

3 of 7 tasks complete
@GianlucaGuarini
Member

I think this feature will be removed because it causing problems in riot-route riot/route#63. I will remove it from the next riot release making riot-observable faster and more reliable. Thanks for your contribution anyway

@Ke-
Contributor
Ke- commented Jul 24, 2016

Why not just add a switch / option to disable namespacing?

I don't mind adding that.

On Mon, Jul 25, 2016 at 9:05 AM Gianluca Guarini notifications@github.com
wrote:

I think this feature will be removed because it causing problems in
riot-route riot/route#63 riot/route#63. I
will remove it from the next riot release making riot-observable faster and
more reliable. Thanks for your contribution anyway


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#15 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AFSWde5K6vRJtj8kJtpIQqnuwkVhnFc5ks5qY9N4gaJpZM4H4jss
.

@Ke-
Contributor
Ke- commented Jul 24, 2016

Never mind; just reviewed the performance numbers.

On Mon, Jul 25, 2016 at 9:46 AM Ke Carlton ke@zeroloop.com wrote:

Why not just add a switch / option to disable namespacing?

I don't mind adding that.

On Mon, Jul 25, 2016 at 9:05 AM Gianluca Guarini notifications@github.com
wrote:

I think this feature will be removed because it causing problems in
riot-route riot/route#63 riot/route#63. I
will remove it from the next riot release making riot-observable faster and
more reliable. Thanks for your contribution anyway


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#15 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AFSWde5K6vRJtj8kJtpIQqnuwkVhnFc5ks5qY9N4gaJpZM4H4jss
.

@GianlucaGuarini
Member
GianlucaGuarini commented Jul 24, 2016 edited

@Ke- this was an awesome feature but it was a breaking change that was introduced in a minor release so I will need to remove it also in the next riot@2.6.0 release. I am sure once riot@3.0.0 will be out we can still discuss about this feature ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment