breaks are transpiled to return inside then

Current Behavior

While working on my code I noticed a bug that led to my transpiled files and I saw that tsdx was incorrectly transforming my break inside switch cases to return.
I thoughts that was rather odd so I created a sample project with the following code to pinpoint the issue:

function wait() {
  return new Promise((resolve, reject) => {
    setTimeout(() => {
      resolve();
    }, 1000);
  });
}

export const test = async () => {
  await wait();
  switch ('' as any) {
    case 'a':
      break;
    case 'b':
      break;
    default:
      break;
  }
  const foo = { bar: true };
  console.log('foo :', foo);
};

When running tsdx build, this is what I get:

'use strict';

Object.defineProperty(exports, '__esModule', { value: true });

function wait() {
  return new Promise(function (resolve, reject) {
    setTimeout(function () {
      resolve();
    }, 1000);
  });
}

var test = function test() {
  try {
    return Promise.resolve(wait()).then(function () {
      switch ('') {
        case 'a':
          return;

        case 'b':
          return;

        default:
          return;
      }

      var foo = {
        bar: true
      };
      console.log('foo :', foo);
    });
  } catch (e) {
    return Promise.reject(e);
  }
};

exports.test = test;
//# sourceMappingURL=switch-test.cjs.development.js.map

As you can see, inside test, breaks inside then are transformed to returns, which is NOT correct. The console.log line would never be reached.

Expected behavior

breaks inside then should stay breaks.

Your environment

Software	Version(s)
TSDX	0.12.3
TypeScript	^3.7.5
Browser
npm/Yarn	yarn 1.19.1
Node	10.16.3
Operating System	Linux, but same behavior on windows

So, seems like this is due to rpetrich/babel-plugin-transform-async-to-promises#49.

Can tsdx move away from babel-plugin-transform-async-to-promises?

@CyriacBr

Thanks for tracking down the root cause @CyriacBr ! That certainly seems problematic 🙁 As far as I know, TSDX uses it because it is a syntax transform instead of a runtime polyfill, therefore no need to use regenerator-runtime.

There are some alternatives, like fast-async, but that seems less used and less maintained 🙁

I'm not 100% sure if there's a way to disable it with a custom babelrc (it is merged with TSDX's normally, would have to look into that). You might be able to make some small modifications to your code in the meantime to workaround that bug, but idk

For now I'm going to override the rollup config and only use @rollup/typescript for compilation and transpilation.

Ok sure, overriding rollup-plugin-typescript2's target is an option as well. TS does use a generator polyfill however -- see microsoft/TypeScript#31621 about transpiling to promises instead.

Can also override rollup-plugin-babel to stay closer to TSDX. Can see the plugins used internally here.

I'm looking to add a Babel preset to help make customizing these types of things easier, #383 has some progress on that.
Removing an option is a the difficult part here though; there's this old issue about Babel not allowing you to disable a plugin within a preset (different from how ESLint works), but maybe that's no longer true with overrides. Haven't faced this issue before (first time reading about overrides etc), sorry I can't be more helpful on that front.

Alright. For the time being I ended up forking TSDX and removed anything babel related. If anyone passing by and encountered the same issue, you can thing my fork here, and it's published. I'm fine with TS's runtime polyfill.

TSDX has been my go-to initializer for many months now, thanks a lot for setting up that.

Ok, I don't normally recommend forking and the other maintainers recommend using patch-package for internal changes.

I'm also pretty sure this can be worked around with just a tsdx.config.js like I said above (that's what I thought you were going to do too), either overriding rollup-plugin-babel's config (that way you can remove plugins) or overriding rollup-plugin-typescript2's config to change target to something lower.

I'm also pretty sure this can be worked around with just a tsdx.config.js like I said above (that's what I thought you were going to do too)

Yes, that's what I initially wanted to do, but then I realized I'd have to perform the same process for each new project. That'd defeat the process of using a initializer like TSDX. I wanted something that just works out of the box without me copy-pasting stuff from previous projects.
I'll jump back to tsdx once it is fixed and doesn't need any extra step :).

Yes, that's what I initially wanted to do, but then I realized I'd have to perform the same process for each new project. That'd defeat the process of using a initializer like TSDX. I wanted something that just works out of the box without me copy-pasting stuff from previous projects.

Ah gotcha, that makes sense. Yea I can see why some workarounds can be annoying for people with multiple packages, though you might not need this everywhere.

The other option is to create a shareable tsdx.config.js, like:

// tsdx.config.js
module.exports = require('@cyriacbr/tsdx-config-no-babel')

Though if something like this is officially adopted (I'm planning on making an RFC soon), it would probably look more like a plugin than a shareable config:

// tsdx.config.js
const noBabelPlugin = require('@cyriacbr/tsdx-plugin-no-babel')

module.exports = {
   rollup (config) {
      noBabelPlugin(config)
      // ...
      return config
   }
}

You can of course do either (or both) unofficially though.
The RFC will be to establish a spec for compatibility and to ideally eventually bring some common use cases in as "core" plugins (esp. since they can be brittle). But we'll see how that goes.

I'll jump back to tsdx once it is fixed and doesn't need any extra step :).

Yea, unfortunately since it's upstream and hasn't been responded to in ~3 months it might take a while 😞 . In the meantime, your fork is likely to become out-of-sync, so it becomes a maintenance burden for you to update 😕

Looks like there was another incorrect transpilation brought up from this plugin: #423 / rpetrich/babel-plugin-transform-async-to-promises#53 .

EDIT: Also seems like #190 is related. But that doesn't seem to error if it's used standalone

Would probably be good to consider replacing this, especially since it hasn't been maintained the past few months (although regenerator maybe the only good alternative) 😕

Another comment on that issue revealing more broken code: rpetrich/babel-plugin-transform-async-to-promises#49 (comment). And still no response / maintenance.

At this point, I think it would be optimal to switch to regenerator because at least the code it produces is actually correct and not broken in subtle, hard-to-detect ways. I'm also not sure really how much difference in weight it adds as a polyfill because babel-plugin-transform-async-to-promises also adds helpers, it's not just a polyfill vs. no polyfill comparison.

@agilgur5

Not to just add unnecessary noise, but this drove me crazy! I discovered even more broken examples where the emitted code would literally use things like _break4 or _break2 in a different scope than they were declared, code that was not just incorrect but could be statically determined to be incorrect! Compiling to non-regenerator-runtime code is a noble goal, but ultimately I can't justify using tsdx when it's in this state. It does bother me to have to essentially "eject" because of such a glaring issue when otherwise the project is so useful.

One of the main reasons I'm writing this comment is so people who search something like _break4 or _break2 can find this and see the problem!

Thank you @agilgur5 and @hb-seb for trying to resolve this! I hope it can be resolved soon.

@allcontributors

@allcontributors please add @CyriacBr for bug

@agilgur5

@agilgur5

I've put up a pull request to add @CyriacBr! 🎉

async-to-promises has been replaced with babel-plugin-polyfill-regenerator in #795 and will be released in v0.14.0 soon. It will only pure polyfill generators for targets that need a polyfill according to your browserslistrc or preset-env targets

Navigation Menu

Repository navigation

breaks are transpiled to return inside then #509

Current Behavior

Expected behavior

Suggested solution(s)

Your environment

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

breaks are transpiled to return inside then #509

Description

Current Behavior

Expected behavior

Suggested solution(s)

Your environment

Activity

CyriacBr commented on Feb 12, 2020

agilgur5 commented on Feb 13, 2020

CyriacBr commented on Feb 13, 2020

agilgur5 commented on Feb 13, 2020

CyriacBr commented on Feb 14, 2020

agilgur5 commented on Feb 16, 2020

CyriacBr commented on Feb 16, 2020

agilgur5 commented on Feb 16, 2020

agilgur5 commented on Mar 6, 2020

agilgur5 commented on Jul 19, 2020

schickling commented on Jul 31, 2020

agilgur5 commented on Jul 31, 2020

rivertam commented on Aug 19, 2020

agilgur5 commented on Sep 20, 2020

allcontributors commented on Sep 20, 2020

agilgur5 commented on Sep 20, 2020

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions