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

Possible Optimizations #109

Closed
wants to merge 1 commit into from
Closed

Possible Optimizations #109

wants to merge 1 commit into from

Conversation

bozdoz
Copy link

@bozdoz bozdoz commented Jun 25, 2020

  1. slice().map() creates two new arrays whereas forEach just iterates the existing arrays of handlers

  2. using an initializer for all saves a line where you had all = all || new Map()

Hope this helps! Love how minimal it is! 😄

1.  slice().map() creates two new arrays whereas forEach just iterates the existing arrays of handlers

2. using an initializer for `all` saves a line where you had `all = all || new Map()`

Hope this helps!  Love how minimal it is! 😄
@jaylinski
Copy link
Contributor

Size Change: +26 B (2%)

Total Size: 956 B

Filename Size Change
dist/mitt.es.js 223 B +10 B (4%)
dist/mitt.js 222 B +9 B (4%)
dist/mitt.modern.js 209 B -3 B (1%)
dist/mitt.umd.js 302 B +10 B (3%)

@developit
Copy link
Owner

developit commented Jul 9, 2020

Hiya! I appreciate the extra eyeballs on Mitt! Unfortunately, both these changes are functional differences that break edge cases in the library. I always feel bad being the bearer of bad news like this, so I like to explain the extended details. Here we go!

  • .slice() is required for iterating over events because the list of events can be mutated as handlers are being invoked. As an example, the following code shows how an event handler that removes a listener ends up "shifting" the listener offsets during iteration, causing handlers to be skipped:

    const events = mitt();
    events.on('foo', function A(){
      console.log('A');
      events.off('foo', A);  // invokes `all.get('foo').splice(0, 1)`, shifting all items "left" by 1
    });
    events.on('foo', function B() {
      console.log('B');
    });
    events.on('foo', function C() {
      console.log('C');
    });
    events.emit('foo');  // "A", "C"

    Without needing to check in mitt, here's a reproduction of the core of the issue you can fiddle with in the console:

    var list = [1, 2, 3];
    list.forEach(item => {
      if (item === 1) list.splice(0, 1);  // "the first handler removes itself"
      console.log('item: ', item);
    })
    // logs "item: 1", "item: 3" (2 gets skipped!)
  • default parameter values are only applied when parameters are undefined - currently Mitt allows passing null or false as the first argument, which triggers a fall back to the internally created event handler Map. It's also worth noting that Mitt is primarily shipped and consumed as ES5 (view source), which means 99% of module consumers would end up using a transpiled version of the default argument pattern that is slightly more verbose:

    export default function mitt(all) {
      if (all === void 0) all = new Map;
      // .. etc

    As a fun note: this is one reason I love having the size stats bot comment on PR's. Sadly it isn't able to actually post comments on fork PRs due to a GitHub limitation, but @jaylinski grabbed the output for us here. You can see that the changes does indeed reduce the size of the modern output, however it increases the size of the other (more widely used) outputs by a larger proportion.

@developit developit mentioned this pull request Jul 9, 2020
@developit developit added the can't Out of scope / too large / too seldom-used label Jul 9, 2020
@bozdoz bozdoz closed this Jul 9, 2020
@bozdoz
Copy link
Author

bozdoz commented Jul 9, 2020

@developit Good to know! Yeah I wasn't sure what you were shooting for, so I wanted to de-emphasize the likelihood that this optimizes anything with the word "Possible" in the title. 😄

Thanks for the clarity on the case for the .slice()!

@developit
Copy link
Owner

All good - it's always worth it to revisit decisions :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can't Out of scope / too large / too seldom-used
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants