Skip to content
This repository has been archived by the owner on Jun 10, 2019. It is now read-only.

Support return statements inside switch and try/catch/finally #13

Open
1 of 3 tasks
marten-de-vries opened this issue Feb 3, 2016 · 20 comments
Open
1 of 3 tasks

Comments

@marten-de-vries
Copy link
Collaborator

Currently working in:

  • loops
  • switch statements
  • try/catch/finally

(a.k.a. still some work to do)

@marten-de-vries marten-de-vries changed the title Support return statements properly Support return statements inside switch and try/catch/finally Feb 13, 2016
@nolanlawson
Copy link

To be clear: this is only for returns inside of switches and try/catch/finally, right? Seems to work when you don't return from inside those blocks. (Love this tool, BTW!)

@marten-de-vries
Copy link
Collaborator Author

That's right. If you encounter the issue, you can just work around it by assigning to a variable, then returning that variable after the block. That's pretty much what Kneden would do automatically for you as soon as this is fixed anyway...

@marten-de-vries
Copy link
Collaborator Author

@nolanlawson The newly opened #21 makes me less confident about my previous post. Consider Kneden an alpha-quality tool that will likely require workarounds, for now. So pretty much what the README file says.

@raphaelokon
Copy link

Is the development on this stale? Wondering how I could help out to make it non-alpha.

@marten-de-vries
Copy link
Collaborator Author

@raphaelokon Yes, development pretty much stalled. I would be open for PRs and the like, but currently I don't have the time to work on it myself. That might still change, but no guarantees.

@raphaelokon
Copy link

raphaelokon commented Jun 16, 2016

@marten-de-vries Thanks. I will give it a go and see if I can get the desired behavior implemented. kneden is by far the most readable and concise solution re async/await transpilation.

Are there already tests added that describe the two subtasks mentioned in this issue?
I can see all tests passing so far.

@timdp
Copy link

timdp commented Aug 26, 2016

As an attempt to reignite this project ... To handle return inside try/catch/finally, my gut feeling would be to replace try { } blocks with _try((_return, _catch, _finally) => { }) and then replace every return inside with a call to _return that takes care of the state. I'm guessing that's already been tried though. What are the main issues? Perhaps someone can weigh in?

@ljharb
Copy link
Member

ljharb commented Aug 26, 2016

Would my proposal and shim help here? https://npmjs.com/promise.prototype.finally

@ljharb
Copy link
Member

ljharb commented Oct 9, 2016

@marten-de-vries are you still maintaining this? Airbnb would love to use it, but it either needs to handle all the edge cases, or throw early when attempting to transpile them, so that there's zero chance that incorrect code can make it to production.

@marten-de-vries
Copy link
Collaborator Author

@timdp Don't forget that returns also change the control flow. Still, something like that might work, and if it works I'd be fine merging something like that.
@ljharb Quite possible that would make it easier. Someone would need to try, though. As of currently, no, I'm not working on this, and chances of me picking it up again get increasingly smaller. Sorry. Implementing some easier cases and throwing on the edge ones might be easier, but again, someone would need to try it.

@trusktr
Copy link

trusktr commented Jul 28, 2017

@ljharb Can you convince AirBnB to pay for someone to fix this (at least by adding build-time failure)?

@ljharb
Copy link
Member

ljharb commented Jul 28, 2017

I will definitely work on it!

Is there anyone who could work on it (in a reasonable amount of time) for whom a reasonable amount of money would enable them to do it? If so, please email me!

@swyxio
Copy link

swyxio commented Oct 31, 2017

@ljharb did this get anywhere in the past 3ish months? thinking about taking it up

@loganfsmyth
Copy link
Member

Anyone know if fast-async handles this case? kneden is mostly unmaintained so I'd generally recommend fast-async if you're looking alternatives to async-to-generator.

@ljharb
Copy link
Member

ljharb commented Oct 31, 2017

No, I haven't had time to work on it; anyone should please feel free to start.

fast-async is primarily meant for nodent, so I'd actually prefer to see this one fleshed out and tested for compliance, rather than trying to figure out all the ways fast-async isn't compliant :-/

@swyxio
Copy link

swyxio commented Nov 1, 2017

so I feel completely unqualified to try this since i've never written a babel plugin and usually go with babel-preset-stage-x. but here goes.

it looks to me like we should try to generate tests that model what we think should happen and does not yet happen. right now we have test cases that cover switch and try-catch-finally but evidently they aren't well written since they pass. I'm not clear what the precise issue is since i am a latecomer to this. i see that marten handled return for loops here but i actually don't know if that totally solves it?


switch

i'm just going to focus on switch for now else i feel a bit overwhelmed.

so in expected.js, (corresponding actual file here) the proposed transpile target is evidently wrong as the promise chain will just keep going.

i like the simplicity/readability of the _brokenOut method and i wonder if a really dumb really easy fix is just to have a _hasReturned flag applied to everything following the return statement. would that be too spammy? any other high level approaches i'm not thinking of?

@timdp
Copy link

timdp commented Nov 1, 2017

@loganfsmyth At our company, we've been using fast-async as a replacement for kneden for a while now. I'm pretty sure it handles this case, because otherwise, we would've run into it already. Someone should still check but it's definitely a lot more stable.

@swyxio
Copy link

swyxio commented Nov 1, 2017

that discussion (and it is a worthwhile test to include) should probably happen on fast-async then? the people still interested in kneden are probably people who have decided against using fast-async for one reason or other

@mikirejf
Copy link

mikirejf commented Jan 10, 2018

Does it make any sense to resume the development of this plugin? There is also one more problem with kneden. As referenced in #120, knedendoesn't follow the control flow of the original function.

Fixing that would require quite a design change and I don't think that is worth it, as fast-async/nodent handles that also.

@nicolo-ribaudo
Copy link
Member

Does it make any sense to resume the development of this plugin?

You can use https://github.com/MatAtBread/fast-async

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants