Add full support for generator functions

[cfuehrmann]

This PR was triggered by a need to use generator functions from TypeScript. That's possible for target ES6, but not ES5. To make things work in ES5, I added "babel-loader" to the TypeScript pipeline. Plus, "babel-polyfill" had to be added to make generator functions work in Internet Explorer.

To see that this works, create a new app with the create-react-app command, and change the App.tsx like below. Then one can see that the spread operator works even in Internet Explorer. (Certainly in IE 11.)

[DorianGrey]

This PR was triggered by a need to use generator functions from TypeScript. That's possible for target ES6, but not ES5

This should be supported down to ES3 using the downlevelIteration flag - since TS 2.3:
https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#typescript-23

Have you tried that flag? And if so, which problems occurred?

[cfuehrmann]

@DorianGrey I tried downLevelIteration. But it didn't work. My genFunc example above, on yarn start, produces the following error in IE 11. (The production build produces an obfuscated version of the same error.)

If I use a for ... of ... instead of a spread operator to iterate over the generator, IE 11 acually hangs and asks after a while if it should end the long-running script.

Maybe the problem is related with the following sentence on the TS 2.3 page:

Please note that this requires a native Symbol.iterator or Symbol.iterator shim at runtime for any non-array values.

[DorianGrey]

Ah, I see - Symbol.iterator is not available by default in IE11.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/iterator

The error message indicates an object containing everything that an iterator contains,

The babel polyfill includes a shim for this, aside from (potentially) others. Would you mind testing another shim like the one from core-js? (in conjunction with the downLevelIteration flag)
https://github.com/zloirock/core-js/blob/master/es6/symbol.js

Just want to make sure that there is no "simpler" way than integrating babel for this - until version 7, this would cause a quite unlikely negative performance impact for mid- to large-sized projects.

[cfuehrmann]

@DorianGrey I'll look into it. (I'm super busy this week though, so this might take a while.)

[cfuehrmann]

@DorianGrey Okay, I've enabled downlevelIteration, and replaced "babel-polyfill" with "core-js/es6/symbol". My generator example works in IE11 now. The production bundle of my minimal app had 120KB initially; it had 206KB with "babel-polyfill", but it has only 130KB with "core-js/es6/symbol" :) So your suggestion rocked. Generator functions will use the slow ES5 implementation in all browsers - but at least we have generator functions.

We could now switch the TypeScript target back to ES5, and remove Babel from the TypeScript pipeline. Interestingly, that slightly increases the bundle size from 120KB to 125KB. I'm not sure what the pros and cons are for having Babel in the TypeScript pipeline.

[DorianGrey]

Glad it worked - seems that babel-polyfill attempts to include by far more polyfills than required in this case. The only reason that downlevelIteration is off by default is that it not only affects generator functions, but iterators as well. The resulting code will always be a bit larger and slower, yet more compatible. As you already recognized, most browsers out there already support (at least partially) support iterators and symbols, so using this option was only required for the generator function in conjunction with the spread operation.

that slightly increases the bundle size from 120KB to 125KB

Seems there are differences in how the code is transpiled, and how the required helper functions a picked up. By default, typescript does not pick them up from the external tslib, but puts them into every included file separately. Using tslib for the helper functions has to be configured manually, but in most cases saves some KB.

I'm not sure what the pros and cons are for having Babel in the TypeScript pipeline.

The major downside - at least for larger projects - is the increased (re-)build time, since the whole code has to be processed twice. On the other hand, babel has a lot more plugins / transforms available, quite a lot of them providing features that typescript can't handle (at least for now).
Suppose things will be a lot easier for this setup with babel 7, which supports a preset / transform for typescript, thus avoiding the duplicated processing part - even though it requires to set up type-checking externally. But there are already plugins for webpack to do that job.
Until v7, I would only recommend to go with a chain of typescript and babel if particular features of the latter are required - the additional overhead in build time and project complexity isn't worth it otherwise, I'd say.

[cfuehrmann]

@DorianGrey I've just removed Babel from the TypeScript pipeline. The build time on my local machine went down from 13s to 12.2s.

I'm actually involved in a 50kloc Angularjs project whose where folks just added Babel to the typescript pipeline. I'm not sure about the build times, but they didn't get much worse I think. The people responsible for the change didn't give a clear list of benefits. But one benefit was that certain unit test are now running in Phantom. Of course, Phantom is irrelevant for React.

I'm now in charge of an upcoming major React project, and my hope is to avoid ejecting as long as possible - to avoid needless maintenance and focus on actual features. That's the deeper reason why I made this fork. I can't tell yet if there will be a reason to re-add Babel to the TypeScript pipeline.

May I ask: do you have the rights to merge PRs here, or have you just been a good sport?

[bootstraponline]

CRA upstream is babel based. I'm not sure there's a good reason to remove babel. There are plenty of reasons to use babel. Phantom JS is EOL.

[DorianGrey]

May I ask: do you have the rights to merge PRs here, or have you just been a good sport?

No, I've just contributed some stuff and take a look at some issues and PRs when possible - I'm using this fork personally, thus I'm interested in its progress, and attempt to help out where possible.

I'm not sure about the build times, but they didn't get much worse I think.

It always depends on the particular setup, LoC, code complexity, caching configuration, etc. In most projects I've tried a chain of babel and typescript, the (re-)build performance was approx. 20-30% worse than with just using typescript, so I'm always a bit sceptical about it. Though this does not necessarily mean it will be as worse as that in your project.

and my hope is to avoid ejecting as long as possible

Since the plugin system is not yet available, you might quickly face issues or requirements that will force you to do so - e.g. using a particular loader or webpack plugin. However, maintaining an ejected project isn't that heavy in maintenance - I'm using these myself, and the average update took less than 10 minutes for the last versions: Changes to the setup mostly affect the webpack configuration, which is (in general) easy to merge with its updated counterpart.

[cfuehrmann]

@bootstraponline To avoid misunderstanding: the repo to which this PR belongs has Babel, but it doesn't use Babel to postprocess what comes out of the Typescript compiler. So the discussion here isn't about removing Babel. It's about whether to add Babel as a backend to Typescript.

[cfuehrmann]

@DorianGrey Thanks for those helpful infos! My apprehension about maintaining an ejected project isn't about updating the ejected packages via yarn; what I'd like to avoid is loosing touch with updates to CRA and failing to benefit from improved solutions there.

[DorianGrey]

@cfuehrmann :

what I'd like to avoid is loosing touch with updates to CRA and failing to benefit from improved solutions there.

Even without ejecting the project, you will still have to deal with updates to files generated for your project, e.g. the service worker registration script. As long a you don't reorganize the ejected structure completely, updates are simple to handle:

• Update CRA.
• Generate a new project using CRA.
• Eject it.
• Port the diff to your current project.

The last merge on one of my playground projects was this one:
DorianGrey/react-ts-playground@d444567

Only took a couple of minutes. Main purpose for ejection in my case were several plugins to use and the modification of some loader-chains. I also deduped the webpack configs a bit, since there are a lot of duplicates in dev vs. prod config in upstream. Still thinking about using webpack-blocks for further optimizations on.

[cfuehrmann]

@DorianGrey Very helpful suggestion, thanks!

[wmonk]

@cfuehrmann did you original issue get fixed by updating your tsconfig?

[cfuehrmann]

@wmonk Note quite. To make my generator-function example work in IE11, I also had to add the shim "core-js/es6/symbol" to the two webpack configs. But that, together with "downlevelIteration" in the tsconfig, sufficed to solve my issue.

[DorianGrey]

Even though generators are not that uncommon, I'm not sure if using the polyfill and the compiler option should be the default. Both increase the resulting bundle size, and the compiler option has a negative impact on the runtime performance.

However, the various babel plugins that CRA uses by default (see config) seem to have an equivalent effect, which would tend towards using these changes to provide an equivalently result (checked that on a fresh CRA project). In that case, I'd suggest to add this polyfill to the polyfills file instead of the separate entry points - that's easier to maintain. Possibly behind a condition like

if (process.env.NODE_ENV !== 'test') {
  require('core-js/es6/symbol');
}

since node already supports this for a while now (iirc), so it's not needed for testing.

[cfuehrmann]

I agree about the increase in bundle size - though that's "only" around 8% (from 120KB to 130KB). As for the runtime performance, I see no downside: before the change, generator functions didn't work at all (because of the ES5 targeting without downlevelIteration), and now they work - just not at ES6 speed. So, no previously working feature got slowed down. Or am I missing something?

[DorianGrey]

The runtime performance gets a bit worse because of additional helpers used and introduced, but I have to admit that I'm not sure about concrete numbers on how bad the would be. However, the generated code would not be slower than its counterpart generated by babel in CRA, which essentially does the same.

Thus, I agree that feature parity should be favorable in this case.

[cfuehrmann]

Synced with upstream. And I just found out: if a user wants full generator support for IE11 without the changes in my PR, they could also add import 'core-js/es6/symbol' as the first line of their index.tsx, and --downlevelIteration=true in their tsconfig.json. Incidentally, I think this business might matter more soon, since this kind of generator support is needed for the increasingly popular redux-saga.

[DorianGrey]

I'm fine with this semantically, just stated my last comment in an explicit in-line review.

Only some structural stuff.

[DorianGrey]

I'd favor to add this polyfill to the general polyfills files instead of this array, esp. regarding maintenance. Would be good to have some kind of comment or a short description of why this is included.

[DorianGrey]

Same here.

[cfuehrmann]

@DorianGrey I don't know from which repo these polyfills come. Who could add "core-js/es6/symbols" to them?

[DorianGrey]

The polyfills file referenced in these configs is this one (it's picked up via a relative path by them):
https://github.com/wmonk/create-react-app-typescript/blob/master/packages/react-scripts/config/polyfills.js
So just put the require statement for core-js/es6/symbol there, including the comment about its sense resp. use as mentioned above.

core-js is already a transitive dependency introduced by react-scripts-ts#ts-jest#babel-core#babel-register resp. react-scripts-ts#ts-jest#babel-core#babel-runtime, so its availability can be assumed.

[cfuehrmann]

Sorry for the long silence. My immediate problem has been solved, by simply importing "core-js" from the JavaScript entry point, "index.js". I'm now no longer sure if baking in (a superset of) the required polyfills is the right way to go. As far as I'm concerned, this PR can be closed. Shall I close it?