Optimize allocations

[armanbilge]

• avoids unnecessary wrapping of Runnable as () => Unit
• replaces Scala Map and Option-based get with an ordinary js.Object

[sjrd]

This is going to reach ju.HashMap for every Scala.js codebase that uses this library, potentially increasing code size. Have you considered using a plain old JS object instead?

[armanbilge]

Good point, that's what the upstream does.
https://github.com/YuzuJS/setImmediate/blob/f1ccbfdf09cb93aadf77c4aa749ea554503b9234/setImmediate.js#L9C9-L9C22

[sjrd]

I'm pretty sure you need tasksByHandle.selectDynamic(handle) here. As is, you're calling tasksByHandle with handle as an argument.

[armanbilge]

Yes, you're right ... messed up the refactor from an initial try with @BracketAccess. But selectDynamic requires a string, so going to try @BracketAccess again

[sjrd]

If you're going to cast here anyway, using js.UndefOr only to get isDefined is more trouble than it's worth. You can use js.isUndefined(task) instead.

[sjrd]

And here (and below as well) you'll need updateDynamic.

[sjrd]

With tasksByHandle(handle) now returning a Runnable, there will be an implicit cast to Runnable here that will CCE when it's undefined.

With a custom TaskMap (which really makes sense, btw), you'll get a better mileage by having the apply return a js.UndefOr[Runnable]. Then you could actually just call foreach on it, and you won't need the cast further down. Seems like a win overall.

I wonder how we could better test that each code path actually works. The code paths that are polyfilling really old stuff are under-tested, it seems.

[armanbilge]

I wonder how we could better test that each code path actually works. The code paths that are polyfilling really old stuff are under-tested, it seems.

Actually, I think the reason we are not hitting the CCE in the undefined case is because it's impossible for us. I don't think we need to consider this case at all.

It was cargo-culted from the upstream, but the upstream also implements clearImmediate.
https://github.com/YuzuJS/setImmediate/blob/f1ccbfdf09cb93aadf77c4aa749ea554503b9234/setImmediate.js#L31-L33

[sjrd]

Ah ah! That makes sense. In that case the logic looks good now. We should rename runIfPresent to run, though. Otherwise the next poor soul coming here will be confused.