search

jaredpalmer / tsdx

Zero-config CLI for TypeScript package development
https://tsdx.io
MIT License
11.28k stars 507 forks source link

Use regenerator instead of async-to-promises to transpile async functions #795

Closed hb-seb closed 4 years ago

hb-seb commented 4 years ago

Reason for this change: Ran into a variety of transpilation issues that surfaced after migrating existing code to tsdx, ranging from functions resolving with wrong values, loop iterations not being executed, and code after return statements being run.

Seeing that there are already other known issues with the (unmaintained looking) library, and the bugs are hard to isolate and behave differently in automated test environments, only replacing the transpilation library for a more robust solution would give me peace of mind.

Unfortunately doing so from the outside currently seems impossible, without replacing rollup plugins as a whole, so a fork seemed more reasonable. Maybe this PR could help to fix this issue at its core.

Changes:

vercel[bot] commented 4 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/formium/tsdx/mza5yccgm
✅ Preview: https://tsdx-git-fork-hb-seb-fix-async-transpilation.formium.vercel.app

agilgur5 commented 4 years ago

So I did some research in Babel to see if there had been any progress on the issue of library polyfills in recent time and there actually has been! It doesn't totally resolve all the issues with library polyfills but should help this move forward

https://github.com/babel/babel/issues/6629 was recently closed and babel-polyfills was released. So by using babel-plugin-polyfill-regenerator, we don't need to turn on useBuiltIns of preset-env nor need to pollute the global env in doing so. Instead:

plugins: [
  // ...
  ['polyfill-regenerator', {
    method: 'usage-pure',
    targets: customOptions.targets
  }]
]

That should be functionally equivalent to async-to-promises then if not a little better. We can remove the more disruptive core-js piece then and defer that work till a later point once there's a more optimal solution for that (probably would be having it as optionalPeerDependencies and warning the user to include as their own library's peerDependencies if we detect it's used, same could be done with regenerator-runtime).

cyri113 commented 4 years ago

any idea on when this PR will be merged?

agilgur5 commented 4 years ago

Since this has been stale for several weeks now and is a high priority item (and the main item left for v0.14.0), I'll be taking over this PR now to get it over the finish line

agilgur5 commented 4 years ago

Was going to add a test, but I see a test is failing here because I forgot I added an integration test in #627 for ['@babel/plugin-transform-runtime', { helpers: false }] in order to ensure that polyfills could be added for generators (workaround for #547 / #169). May move this test elsewhere and remove the transform-runtime piece since babel-polyfills etc should be used and generator polyfills are now supported out-of-the-box with this PR. But probably want to make this test pass first so it's not horribly breaking to transform-runtime users.

But this error is a bit cryptic 😕 ... and no files are output for me to inspect 😕 :

Error when using sourcemap for reporting an error: Can't resolve original location of error.
Error when using sourcemap for reporting an error: Can't resolve original location of error.
Error: 'default' is not exported by ../node_modules/regenerator-runtime/runtime.js, imported by src/generator.ts

at /Users/antongilgur/Desktop/GitHub/oss/tsdx/stage-integration-build-withBabel/src/generator.ts:1:7

1: export function* testGenerator() {
          ^
2:   return yield 'blah';
 FAIL  test/integration/tsdx-build-withBabel.test.ts (8.222s)
  integration :: tsdx build :: .babelrc.js
    ✕ should add an import of regeneratorRuntime (5718ms)

Default import is the issue? Can't tell if this is because of the external/bundling piece, a bug in polyfill-regenerator, or something else (Rollup multi-output? Babel plugin order?)...

EDIT: seems to error out even if I remove transform-runtime from the integration test's custom Babel config 😕 😕

agilgur5 commented 4 years ago

~Crap, seems like this might be a bug upstream (which would make a fix take longer) 😕
polyfill-regenerator calls a function named injectDefaultImport, which is actually defined up one more level upstream... Hmmm... but it has a test for this so maybe the problem is actually later in the build chain...~ 🤔 ~

EDIT: or not... the error seems common: https://github.com/rollup/rollup-plugin-commonjs/issues/361 and explained well here: https://github.com/wessberg/rollup-plugin-ts/issues/78#issuecomment-583476953. So I guess the rollup-plugin-commonjs modification that OP made for core-js will also need to apply here 🤔 I also tried transpiling to only esm but that got the same error 🤔 ... need to dig in a bit to understand what I'm missing about plugin-commonjs usage and usage with non-cjs (since Rollup should understand ESM)

agilgur5 commented 4 years ago

Ah, rollup-plugin-commonjs is for transforming CJS modules to ESM (not the other way around) so that Rollup can understand them (Rollup only knows ESM without plugins). regenerator-runtime (and many packages) is CJS, so needs to be transformed.

Got tests to work by using the same config from OP for core-js but instead for regenerator-runtime. But have been testing targets and haven't been able to get it to not bundle regenerator-runtime... 😕 Also ran into this bug: https://github.com/babel/babel-polyfills/issues/35 which could make inheriting @babel/preset-env's targets a bit tougher (related: https://github.com/babel/rfcs/pull/2)

agilgur5 commented 4 years ago

Ah... well targets isn't working because it apparently isn't supported by polyfill-regenerator 😕 This line needs to specify targets per the polyfill-provider docs.

This shouldn't be too hard to add though fortunately. Will file an issue now and maybe PR tomorrow. EDIT: see https://github.com/babel/babel-polyfills/issues/36

agilgur5 commented 4 years ago

~Added a regression test for #190 so can confirm this also fixes that issue now. #190 was different from the other async-to-promises issues in that it wasn't a correctness issue but an actual Babel plugin error, and per OP there, potentially due to interaction with other plugins in TSDX~

EDIT: I failed to reproduce #190 with TSDX v0.13.3 actually per https://github.com/formium/tsdx/issues/190#issuecomment-694739974, so that one seems to already have been fixed by other changes (it's from TSDX v0.9.0).

I was able to successfully reproduce #869 though, which has a different error but is similarly a "error not correctness" bug upstream in async-to-promises. Or well, it produces invalid JS so it's actually both correctness and error. Adding a regression test for this.

agilgur5 commented 4 years ago

Added a regression test for #869

Also https://github.com/babel/babel-polyfills/issues/36 was responded to, so I've added a test against targets that ensures regeneratorRuntime isn't inserted in environments that already support generators.

All tests pass. This is ready for review now

agilgur5 commented 4 years ago

@allcontributors please add @hb-seb for code

allcontributors[bot] commented 4 years ago

@agilgur5

I've put up a pull request to add @hb-seb! :tada: