Transformed all remaining occurences of for..of to forEach()

[rhettlivingston]

No description provided.

[benmosher]

fixes #415?

[benmosher]

this would ideally be a some so that it short-circuits. not sure how to achieve with a Set...

[rhettlivingston]

I learned while doing this that Maps and Sets are very fundamentally not Arrays. So, no some and not even a clear way to utilize the basic for statement and break out of it (i.e. you can't access them in an indexed fashion).

I suspect the overhead of converting to an array just to use some would be greater than the gain.

[rhettlivingston]

After thinking about this some,,, I may experiment with directly accessing the iterator in a for loop. This is essentially what is going on with for..of, but I might be able to get away with it if I don't involve the underlying Symbol type and go directly to the Map / Set.

I've also realized I might have changed more than I had too. The instances that definitely caused problems involved using for..of to iterate Maps. In response, I changed all occurrences of for..of. It is possible that using for..of to iterate a Set causes no issue.

[rhettlivingston]

Yes. Didn't put that in the comment because I was merging into the branch
that fixes it.

I have to apologize for what it does to your code. I really wish I knew how
to just make the for..of implementation play well with others. forEach is
very inferior in this case because you can neither 'continue' within it nor
'return' from it.

On Thu, Jul 7, 2016 at 1:15 PM, Ben Mosher notifications@github.com wrote:

fixes #415 #415?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#426 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AH5TYs2rKotZaL09yo00Yrz6DpjeAPWvks5qTTRDgaJpZM4JHTZ0
.

[benmosher]

same here, ideally would short-circuit

[benmosher]

On short-circuits: I think you're probably right, best to leave it be.

Though notably, this.dependencies only ever deals with its values after parse time, so the only use it gets being a Map is one-per-path export * from x behavior, which could be easily achieved with an ephemeral Set of paths during parse and just if (!exportAllPaths.has(remoteMap)) m.dependencies.push(() => ExportMap.for(remoteMap, context) on line ~163 of getExports.js.

FWIW, I'll probably revert to the for ... of syntax in v2 anyway since 0.10 will no longer be supported. so no sweat either way. 😁 Best to just get it correct for the short term and not worry too much about the ideal case.

I'm just over-thinking it.

[benmosher]

I'll go ahead and merge as-is if it fixes #415, no need to sweat the style stuff ATM.

[rhettlivingston]

Thank you very much for accepting this. And I'd absolutely agree with reverting to for..of later. It is unquestionably the proper way to traverse Maps and Sets.