refactor: fixes #2, #6, #12 and #13 #19

tunnckoCore · 2017-01-17T16:21:46Z

fixes Star handlers called without the event type as the first arg #2
fixes Deduping handler registration #6
fixes Support chaining #12
fixes Support emitting multiple arguments #13
fixes Should the "*" event handlers be fired last? #26 and fix expose all #27 too
no more accepts first argument (seen in the smaller branch?), but we can back with cost of 8 bytes and a small tweak in tests
bonus: can access object of the all handlers through emitter.all
exactly 200b (with current master build process)

Sorry for the yarn.lock, spaces and semicolons. It seems eslint --fix does nothing with that recommended preset.

update (17 jan, 18h):

initially was with toLowerCase, but dropped .toLowerCase()
initially was with Set, but dropped and switched to native array

update (18 jan, 03h):

drop Support emitting multiple arguments #13 - not needed really

tunnckoCore · 2017-01-17T16:26:52Z

example

'use strict'

var mitt = require('./dist/mitt')
var ee = mitt()

ee
  .on('*', (type, arg) => console.log('wildcard:', type, arg))
  .on('foo', (one, two, three) => console.log('foo1:', one, two, three))
  .on('foo', (one, two, three) => console.log('foo2:', one, two, three))
  .on('bar', (arg) => console.log('bar:', arg))
  .emit('foo', 1, 2, 3)
  .emit('bar', 444)

console.log(ee.all)

var emitter = mitt()

console.log(emitter.all) // => {}

emitter.emit('foo', 777)

dotproto · 2017-01-17T16:27:59Z

FYI, this PR doesn't follow the indentation style of the original file (tabs). You might want to clean up the formatting to better match the original.

tunnckoCore · 2017-01-17T17:01:19Z

That's because there's no .editorconfig in the root and it would be fixed when he push it. The only way is to push some editor config and fix it to the original.

developit · 2017-01-17T17:45:56Z

The .editorconfig I'll be adding is here by the way.

developit · 2017-01-17T17:48:46Z

TBH I don't think we can merge anything relying directly on Set. There are other EventEmitter implementations that use that approach, this one is sortof meant to be near-universally compatible (it saddens me to even consider calling it IE9+, I would rather be able to say "it runs in any JS runtime").

developit · 2017-01-17T17:49:48Z

src/index.js

-		emit(type, event) {
-			list('*').concat(list(type)).forEach( f => { f(event); });
+		emit(type, arg1, arg2, arg3) {
+			list(type).forEach((handler) => handler(arg1, arg2, arg3));


dropping the braces in loops here will insert an unused return statement.

Hm. Will try it.

Don't know but it adds 2 bytes 😆

funky! I never thought to check - I guess that's another one for the "gzip is cool" book haha

tunnckoCore · 2017-01-17T17:55:30Z

@developit 1) this editor config these not have ident size for js files. But as you want.

Dropping Set it goes ~230+ bytes somehow. I can try again later ;d

developit · 2017-01-17T17:59:39Z

that editorconfig sets tabstops to "tab" here for all files.

tunnckoCore · 2017-01-17T18:00:22Z

So yea, if we consider 1) dropping toLowerCase(); 2) dropping Set and 3) emitting 2 arguments instead of 3 arguments, then we finish with 201b ;d

I switch it and can push if you want.

developit · 2017-01-17T18:02:36Z

Seems like a good update. I'd like more feedback around multiple arguments before we merge that though. Any idea if the way you've attached all is smaller or larger than the old argument approach? (sorry for all the questions haha)

tunnckoCore · 2017-01-17T18:05:36Z

Any idea if the way you've attached all is smaller or larger than the old argument approach?

just realized that it is the reason it is 201, not 200 as currently :D need to update the ignore case tests

developit · 2017-01-17T18:14:15Z

I'm fine with either approach for exposing all - it's a little sad that we have to do so, but the tests rely on it. Maybe it'll be something we compile out?

developit · 2017-01-17T18:36:24Z

Yeah we might want to remove documentation.js output from it I guess.

tunnckoCore · 2017-01-17T18:37:28Z

Hoooray. We got green.

edit: As about the multiple arguments.. It worth nothing. It will lower the size with 3-4 bytes.

developit · 2017-01-17T18:38:55Z

I have never tried multiple arguments with Node's EventEmitter. Not sure if it is supported there.

tunnckoCore · 2017-01-17T18:40:07Z

Yea it supports. I used them few times.

var Emitter = require('events')
var ee = new Emitter()

ee.on('foo', console.log)
ee.emit('foo', 1, 2, 3)

tunnckoCore · 2017-01-17T18:44:43Z

Yeah we might want to remove documentation.js output from it I guess.

You can try https://github.com/verbose/verb/tree/dev + verb-generate-readme. It gives a lot more better and meaningful (readme) docs and scaffolding.

edit: example: https://github.com/tunnckoCore/koa-better-router

developit · 2017-01-17T19:38:38Z

Looks good! I'm pretty used to Documentation.js though, so unless someone PR's it I'm unlikely to scrap and do something new (also seems like it's a little bit in flux). FWIW I think it'd be fine to use documentation.js' built-in support for publishing an HTML webpage and just have that deployed via gh-pages. That would be a nice place for examples and whatnot.

That, or scrap the JSDoc annotations altogether and switch to written documentation, since there are only three methods. Examples and recipes might be more useful than generated docs here.

tunnckoCore · 2017-01-18T00:24:46Z

So, good night from here, let's get rid of some bytes ;p (as per #13 (comment) and #13 (comment))

tunnckoCore · 2017-01-18T00:29:08Z

btw, what you think about the order in .emit?

		emit(type, evt) {
			(all[type] || []).map((handler) => { handler(evt); });
			(all['*'] || []).map((handler) => { handler(type, evt); });
			return ret;
		}

I believe it should emit wildcards first? and should add test for that

tunnckoCore · 2017-01-21T21:57:01Z

@developit okey. So it will remain as current :)

Let me fix the conflict and we could merge?

developit · 2017-01-22T00:26:04Z

I think there's still the open question of this preventing #1

tunnckoCore · 2017-01-22T00:28:48Z

Yea. But I think there are more open issues and wants and PRs for current style/approach (as usual emitter). This PR closes all of them.

But okey, we'll wait :)

developit · 2017-01-22T00:29:36Z

Not necessarily wanting to wait on all of it - we could extract the return this'es and move forward with the rest. That's one painful part of the lib being so small - everything is a merge conflict lol

tunnckoCore · 2017-01-22T01:04:48Z

I don't see for what conflict you're talking about. If some attribution is needed to the others - I believe I did it correct way, if not sorry. I started that PR with merging them (at least for the tests). Then I just start everything in index.js from scratch to get them fit in 200b. It is alsoo adjusted to the comments from the other issues and PRs.

we could extract the return this'es

It's not just that. Initially it was for the #12 (mainly) and #13. But it was not possible to accomplish #12 without some kind of rewriting the whole thing and fit the needs from other PRs + the size limit.

tunnckoCore · 2017-01-22T01:06:15Z

It's just impossible to merge them one by one, because they separately does not fit in 200b limit (almost).

developit · 2017-01-22T15:49:58Z

Ah my merge conflict comment was just more a general comment. You don't think we could just hold off on the return this'es in this PR?

tunnckoCore · 2017-01-25T21:20:06Z

Don't know. Do what you want :) I just invest a bit of time and was excited :) I just saying that I don't see any problems in merging it.

developit · 2017-01-25T22:41:16Z

I might merge but then back out the returns. Will be a bit though. Appreciate all your work on this, sorry for being slow heh

developit · 2017-01-29T16:15:02Z

Hoping to merge this today.

tunnckoCore · 2017-01-29T20:56:45Z

Cool. I'm playing with it few days. For tiny router and tiny store, and actually found https://github.com/tkh44/smitty that gave me some ideas.

developit · 2017-01-30T19:25:05Z

ah nice- you've been playing with the version of mitt from this PR? Good to know it's been used prior to merging.

tunnckoCore · 2017-02-05T10:00:56Z

@developit yep, with that, which is 196 bytes ;d

developit · 2017-02-06T00:47:19Z

Kk.

developit · 2017-02-23T19:22:56Z

@tunnckoCore looking to merge this - can we just double check what the size hit is for the chaining returns? I'm tempted to defer deciding on return values but the rest of this PR is really in need of being merged.

tunnckoCore · 2017-02-23T22:07:22Z

What I see is 13 passing tests, chaining ( #12 ), Object.create(null) ( #11 , #9 ) and 196b 🎉

I kinda still can't believe it :D

tunnckoCore · 2017-02-23T22:10:40Z

Can we merge, and then I can PR for the better rollupconfig and more clean npm scripts using the rollup plugins, so i can be more sure that that is real. Just to proceed and close/clean/fix/resolve most of the opened issues and prs, then we can continue.

developit · 2017-02-24T01:17:32Z

Agreed for sure, just I don't know about the return values. If we merge and release, they are set in stone. Not saying I'm necessarily against chaining, just that it's the only thing in this PR that is causing me to stress :P

tunnckoCore · 2017-02-24T01:32:12Z

@developit why? what's wrong with the chaining, it is pretty useful. Maybe because you .emit from component on-* events haha? You kinda can detect if is-mitt :D like o && o.all && o.emit && o.on?

developit · 2017-02-24T03:14:49Z

My main concerns are that it's not supported in EventEmitter and makes it impossible to support returning an unsubscribe method. I'm not sure about the unsubscribe itself, but it seems a shame to omit that for a sugar syntax in a 200b lib.

developit · 2017-02-25T20:54:06Z

I'm merging, but I'm going to leave all as an argument instead of property and remove the return values (at least temporarily) so this doesn't have to be a major version bump.

refactor: fixes developit#2, developit#6, developit#12 and developit#13

a6190e9

add example.js

8b4f2de

add editorconfig, fix original identation

e7c222c

developit reviewed Jan 17, 2017

View reviewed changes

tunnckoCore and others added 6 commits January 17, 2017 20:26

fix: switch Set with array; update tests; drop toLowerCase()

0c8202b

cleanup

db21ce5

resolve readme conflict

e361ab5

readme, re-run build process

19a32dc

switch to Preact's editorconfig

e814a57

Merge branch 'master' into refactor

9276c3c

tunnckoCore mentioned this pull request Jan 18, 2017

Support emitting multiple arguments #13

Closed

dotproto mentioned this pull request Jan 18, 2017

Support chaining #12

Closed

tunnckoCore mentioned this pull request Jan 18, 2017

chore(editorconfig): add editorconfig #21

Merged

dotproto mentioned this pull request Jan 20, 2017

expose all #27

Closed

Merge branch 'master' into refactor

70e8ad1

tunnckoCore mentioned this pull request Jan 27, 2017

typescript #34

Closed

Merge branch 'master' into refactor

4799316

Update .editorconfig

29c4aa1

developit merged commit 29c4aa1 into developit:master Feb 25, 2017

developit mentioned this pull request Feb 25, 2017

fix: call star handlers with type as first parameter #3

Closed

refactor: fixes #2, #6, #12 and #13 #19

refactor: fixes #2, #6, #12 and #13 #19

Conversation

tunnckoCore commented Jan 17, 2017 • edited Loading

tunnckoCore commented Jan 17, 2017

dotproto commented Jan 17, 2017

tunnckoCore commented Jan 17, 2017 • edited Loading

developit commented Jan 17, 2017

developit commented Jan 17, 2017

developit Jan 17, 2017

Choose a reason for hiding this comment

tunnckoCore Jan 17, 2017

Choose a reason for hiding this comment

tunnckoCore Jan 17, 2017

Choose a reason for hiding this comment

developit Jan 17, 2017

Choose a reason for hiding this comment

tunnckoCore commented Jan 17, 2017 • edited Loading

developit commented Jan 17, 2017

tunnckoCore commented Jan 17, 2017 • edited Loading

developit commented Jan 17, 2017

tunnckoCore commented Jan 17, 2017

developit commented Jan 17, 2017

developit commented Jan 17, 2017

tunnckoCore commented Jan 17, 2017 • edited Loading

developit commented Jan 17, 2017

tunnckoCore commented Jan 17, 2017 • edited Loading

tunnckoCore commented Jan 17, 2017 • edited Loading

developit commented Jan 17, 2017

tunnckoCore commented Jan 18, 2017 • edited Loading

tunnckoCore commented Jan 18, 2017 • edited Loading

tunnckoCore commented Jan 21, 2017

developit commented Jan 22, 2017

tunnckoCore commented Jan 22, 2017

developit commented Jan 22, 2017

tunnckoCore commented Jan 22, 2017 • edited Loading

tunnckoCore commented Jan 22, 2017

developit commented Jan 22, 2017

tunnckoCore commented Jan 25, 2017

developit commented Jan 25, 2017

developit commented Jan 29, 2017

tunnckoCore commented Jan 29, 2017

developit commented Jan 30, 2017

tunnckoCore commented Feb 5, 2017

developit commented Feb 6, 2017

developit commented Feb 23, 2017

tunnckoCore commented Feb 23, 2017

tunnckoCore commented Feb 23, 2017 • edited Loading

developit commented Feb 24, 2017 • edited Loading

tunnckoCore commented Feb 24, 2017

developit commented Feb 24, 2017

developit commented Feb 25, 2017

tunnckoCore commented Jan 17, 2017 •

edited

Loading

tunnckoCore commented Jan 17, 2017 •

edited

Loading

tunnckoCore commented Jan 17, 2017 •

edited

Loading

tunnckoCore commented Jan 17, 2017 •

edited

Loading

tunnckoCore commented Jan 17, 2017 •

edited

Loading

tunnckoCore commented Jan 17, 2017 •

edited

Loading

tunnckoCore commented Jan 17, 2017 •

edited

Loading

tunnckoCore commented Jan 18, 2017 •

edited

Loading

tunnckoCore commented Jan 18, 2017 •

edited

Loading

tunnckoCore commented Jan 22, 2017 •

edited

Loading

tunnckoCore commented Feb 23, 2017 •

edited

Loading

developit commented Feb 24, 2017 •

edited

Loading