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

Fix async recursion #30

Merged
merged 1 commit into from
Mar 14, 2018
Merged

Fix async recursion #30

merged 1 commit into from
Mar 14, 2018

Conversation

Nayni
Copy link
Contributor

@Nayni Nayni commented Mar 13, 2018

Right, so this is becoming a longer story then I initially thought :)

I actually found a proper way to reproduce the issue reported in the webpack plugin: dividab/tsconfig-paths-webpack-plugin#11 you can find it in my own fork. Basically the problem is that if you let the final resolution be a node_module (which was also the case in that issue) the recursion wasn't handled properly.

It took me a while to actually figure out why this was happening but it ended up being an implicit fallthrough that would basically keep the recursion going. I've put a comment in the code to explain it explicitly.

When I fixed this up, the example I made where I experienced the same error as the issue in the webpack plugin also disapeared.

If you'd like the updated example from my fork for future reference and testing, I'm happy to add it as a PR there.

TL;DR: async code with callbacks is hard :)

- The recursion needs to return itself in an explicit else branch and not just a fall-through
@coveralls
Copy link

coveralls commented Mar 13, 2018

Coverage Status

Coverage decreased (-0.4%) to 77.628% when pulling 9685d9a on Nayni:fix-async-recursion into 5a3a57a on dividab:master.

@jonaskello
Copy link
Member

async code with callbacks is hard :)

I could not agree more :-). At some point I was thinking to use async/await instead of callbacks. But I wanted max performance for the webpack plugin and I think using callbacks is slightly faster, at least if you are using a version of node where async/await is not supported natively.

@jonaskello jonaskello merged commit 46ac444 into dividab:master Mar 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants