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

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

Merged
merged 5 commits into from
Sep 20, 2020

Conversation

hb-seb
Copy link
Contributor

@hb-seb hb-seb commented Aug 13, 2020

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:

  • Removed babel-plugin-transform-async-to-promises package and logic associated with filtering it out of preset-env and externals
  • Removed async: false option of the @babel/plugin-transform-regenerator to transpile async functions to regenerators
  • Added useBuiltIns: 'usage' flag to automatically require the regeneratorRuntime
  • Added corejs: 2 flag and the already transitively installed core-js@2 dependency to satisfy a warning that would otherwise appear on every build, stating that core-js should be explicitly installed and the version added to the options when using useBuiltIns to avoid problems when the babel default changes to core-js@3.

@vercel
Copy link

vercel bot commented Aug 13, 2020

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

Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @hb-seb ! Very much appreciate the level of detail you've given in your description and commit messages as well 😄
Per #509 (comment) this is quite wanted given that async-to-promises has been unmaintained for some time now and has correctness issues.

There are some issues with the code you've added though; specifically, libraries use TSDX as a dev dependency, so we can't ensure they have polyfills (like core-js) added as dependencies. This also changes the current behavior of bundling as well. Whether or not libraries should bundle their polyfills is a bit of a debate in the community (personally I think no, that means less code re-use), but as TSDX isn't a dep we can't control that anyway.

Similarly, given the scale of this change and potential for issues like those, we should definitely add tests to this change prior to merging.

src/createRollupConfig.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/babelPluginTsdx.ts Outdated Show resolved Hide resolved
src/babelPluginTsdx.ts Outdated Show resolved Hide resolved
src/babelPluginTsdx.ts Outdated Show resolved Hide resolved
src/babelPluginTsdx.ts Outdated Show resolved Hide resolved
src/babelPluginTsdx.ts Outdated Show resolved Hide resolved
@agilgur5
Copy link
Collaborator

agilgur5 commented Aug 22, 2020

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

babel/babel#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).

@agilgur5 agilgur5 added the version: minor Increment the minor version when merged label Aug 22, 2020
@cyri113

This comment has been minimized.

@agilgur5
Copy link
Collaborator

agilgur5 commented Sep 18, 2020

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
Copy link
Collaborator

agilgur5 commented Sep 18, 2020

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
Copy link
Collaborator

agilgur5 commented Sep 18, 2020

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: rollup/rollup-plugin-commonjs#361 and explained well here: wessberg/rollup-plugin-ts#78 (comment). 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
Copy link
Collaborator

agilgur5 commented Sep 18, 2020

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: babel/babel-polyfills#35 which could make inheriting @babel/preset-env's targets a bit tougher (related: babel/rfcs#2)

@agilgur5
Copy link
Collaborator

agilgur5 commented Sep 18, 2020

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 babel/babel-polyfills#36

- now that generators are supported out-of-the-box via
  polyfill-regenerator, this is no longer an integration test

- also remove @babel/plugin-transform-runtime because it's no longer
  used in tests and likely not needed much either, especially if
  babel-polyfills is recommended for other uses like core-js
  - it no longer ran either since the code was already transformed
    by polyfill-regenerator first
- cover the bases and ensure async is supported too, not just generators

- use some syntax from a bug report that would cause async-to-promises
  to produce invalid JS and Rollup to subsequently error out
- specifically, this checks that `--target node`, which targets Node 10
  right now, does not output regeneratorRuntime as generators have
  native support there (as does async/await)

- remove `targets` from `polyfill-regenerator` config because it's
  actually not necessary for this specific polyfill
  - it only inserts regenerator if a `regeneratorRuntime` global is
    detected, which is only inserted by
    `@babel/plugin-transform-generator` if the `targets` require it
    - meaning that for this particular polyfill, `preset-env`'s
      `targets` are sufficient
@agilgur5
Copy link
Collaborator

agilgur5 commented Sep 20, 2020

Added a regression test for #869

Also babel/babel-polyfills#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

Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, fixing a whole swath of issues with this 🚀

@agilgur5 agilgur5 merged commit 794e5ad into jaredpalmer:master Sep 20, 2020
@agilgur5
Copy link
Collaborator

@allcontributors please add @hb-seb for code

@allcontributors
Copy link
Contributor

@agilgur5

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

Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found some test issues after merging, will have to refactor these in a follow-up PR

test/e2e/tsdx-build-default.test.ts Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
problem: stale Issue has not been responded to in some time topic: async-to-promises Related to bugs with babel-plugin-async-to-promises version: minor Increment the minor version when merged
Projects
None yet
3 participants