Improved looping; safely mutate listeners queue #63
Conversation
crazy4groovy
force-pushed
the
master
branch
5 times, most recently
from
8ddc905
to
4ba58b0
Compare
crazy4groovy
changed the title
crazy4groovy
force-pushed
the
master
branch
2 times, most recently
from
b963778
to
fff2d63
Compare
package.json
Outdated
| @@ -89,7 +90,7 @@ | |||
| "eslint-config-developit": "^1.1.1", | |||
| "gzip-size-cli": "^2.1.0", | |||
| "jest": "^21.2.1", | |||
| "microbundle": "^0.2.4", | |||
| "microbundle": "^0.3.1", | |||
There was a problem hiding this comment.
Needed update to work on Windows
src/index.js
Outdated
| } | ||
| listeners = out; | ||
| const i = listeners.indexOf(listener); | ||
| if (i > -1) listeners.splice(i, 1); |
There was a problem hiding this comment.
This shifts the indices within the Array during event firing, which can cause the wrong set of listeners to be invoked.
There was a problem hiding this comment.
The listeners are called in descending order, so I think the shifted index is irrelevant - the others before it will still be called.
src/util.js
Outdated
| for (let i in props) obj[i] = props[i]; | ||
| return obj; | ||
| // Lighter Object.assign clone | ||
| export function assign(_) { |
There was a problem hiding this comment.
This is expensive: now assign() has variable arity and is unlikely to be optimized.
There was a problem hiding this comment.
It saves bytes compared to its uses.
Also, I'm pretty sure that this is a safe use for optimizations. See https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#what-is-safe-arguments-usage
There was a problem hiding this comment.
On this note, why not just use Object.assign? Support is extremely good: http://kangax.github.io/compat-table/es6/#test-Object_static_methods
There was a problem hiding this comment.
how are you checking the byte savings? we only care about gzipped output size, not the source size. Unistore supports IE9+ so Object.assign isn't available.
There was a problem hiding this comment.
Would you "object" (pun intended) to documenting a need for a polyfill for older (IE <= 11) browser support? Could rip this out altogether.
If not:
I based my saved bytes comment off the fact that assign doesn't need multiple invocations for combining multiple objects, as is its use case in the code. I could test which net-size is smaller between the two ways.
src/index.js
Outdated
| } | ||
|
|
||
| function setState(update, overwrite, action) { | ||
| state = overwrite ? update : assign(assign({}, state), update); | ||
| let currentListeners = listeners; |
There was a problem hiding this comment.
Remove this means that a subscribed function that unsubcribes itself will cause the next listener to be skipped.
There was a problem hiding this comment.
Are you attempting to hedge the case where a listener cb unsubscribes itself, thus modifies the listeners array? I can see your point now. I may write a test case to prove your example.
There was a problem hiding this comment.
Does the new test satisfy your concern now?
src/index.js
Outdated
| for (let i=0; i<arguments.length; i++) args.push(arguments[i]); | ||
| let ret = action.apply(this, args); | ||
| const args = [state]; | ||
| const l = arguments.length; |
There was a problem hiding this comment.
Hoisting this adds bytes and can actually hurt performance.
mraleph has a great blog post describing the effect in detail here:
http://mrale.ph/blog/2014/12/24/array-length-caching.html
There was a problem hiding this comment.
I'll agree with these points.
| let out = []; | ||
| for (let i=0; i<listeners.length; i++) { | ||
| if (listeners[i]===listener) { | ||
| listener = null; |
There was a problem hiding this comment.
Looking at this, I'm not sure what value is added by setting listener to null here. Wouldn't listener just fall out of scope and be gc'ed anyway?
There was a problem hiding this comment.
Setting listener to null prevents the removal of any additional listeners in that loop.
test/unistore.test.js
Outdated
| expect(sub1).toHaveBeenCalledTimes(2); | ||
| expect(sub2).toHaveBeenCalledTimes(1); | ||
| expect(sub3).toHaveBeenCalledTimes(2); | ||
| expect(called).toEqual([3, 2, 1, 3, 1]); |
There was a problem hiding this comment.
I may be testing too much here IMO. Maybe use a sort() or something, just to check calls and not the order they were called.
crazy4groovy
force-pushed
the
master
branch
5 times, most recently
from
0b7f508
to
4be9b4b
Compare
package.json
Outdated
| @@ -28,15 +29,15 @@ | |||
| "bundlesize": [ | |||
| { | |||
| "path": "full/preact.js", | |||
| "maxSize": "750b" | |||
| "maxSize": "727b" | |||
There was a problem hiding this comment.
the other assign function cost an additional 17 bytes, so we should keep the existing one - good call!
crazy4groovy
changed the title
…ners unsubscribing
Admittedly I can't run the tests on my Windows machine :(
So take this PR with a grain of salt.
Error: ENOENT: no such file or directory, open 'D:\dev\projects\unistore\src\integrations*.js'
Error: ENOENT: no such file or directory, open 'D:\dev\projects\unistore\src\combined*.js'
Build output to dist:
554 B: unistore.es.js
323 B: unistore.js
391 B: unistore.umd.js
325 B
ERROR There is no matching file for full/preact.js in D:\dev\projects\unistore
Cheers