Fix flow removal by taking advantage of 'flow' option

[detrohutt]

With the newest version of react-scripts that just released today (2.0.0-next.b2fd8db8), your preset stopped working. I was getting this: Error: "Cannot combine flow and typescript plugins".

I was able to get it working again by making use of the flow option described here. This made the code I removed in my PR unnecessary. However, according to this comment, the flow option wasn't available in babel-preset-react-app until possibly the most recent 1 or 2 beta releases, so I'm not sure if you want to keep the code I removed in there for backwards compatibility with people who may be using your preset with one of the older versions, or just make it a potentially breaking change and recommend that people update to the newest beta release of react-scripts. I have verified that it works with the version of react-scripts released today.

Feel free to make any changes as you see fit. Thanks again for this great preset!

[strothj]

You hit on the reason why I decided against using "flow": false at this time. I don't think there's a good way to pin against the beta releases of react-script. I also wasn't sure when the option was introduced.

I'll be looking over your submission and possible changes soon.

Thanks again for the submission!

[detrohutt]

I didn’t test leaving the redundant code in place but I suspect there wouldn’t be any problem with filtering the presets with the flow preset already removed via the option, so maybe just enabling the flow option without altering any other code would be the most practical (if a bit hacky) solution to keep it both forward and backwards compatible?

[detrohutt]

Ok I tested the updated code in my app and leaving the redundant code seems to work fine as I suspected (with the newest react-scripts still). I still need to test with an older version of babel-preset-react-app to make sure it doesn't throw an error about an unknown option or anything. I'll do that now and let you know what I find out.

[strothj]

I was thinking of taking advantage of Yarn's nohoist feature to create test packages for a couple of the beta versions. I'm really hoping they can get v2 out the door soon though :-)

I see were the old version is having trouble. I'm debating what direction to go with this.

[detrohutt]

Unfortunately I wasn't really able to fully test the older versions of react-scripts with my app because of backwards-compatibility issues and my app is too complex to adjust for the older versions. BUT, I did download and look at the code for each beta release. The flow option is available in every release except 2.0.0-next.096703ab and 2.0.0-next.9754a231 so those are the versions that'd need to be tested against my patch to make sure they don't break from the use of the non-existent flow option. I'll see if I have time to spin up a new create-react-app test app with those versions and set up your preset (with my patch). I'll let you know what I find out.

[strothj]

I'm testing an idea here which I should have ready soon. I'll paste the details here soon.

[detrohutt]

Well I'll just wait to see what you come up with because my testing isn't going very well lol. It looks like I'd need to pin to a bunch of old versions of everything to even get the "stock" app to work with the older react-scripts versions now that newer versions of babel etc. are out.

[strothj]

import presetReactApp from "babel-preset-react-app";

const removeFlowPreset = (...args: any[]) => {
  // Pass along arguments to babel-preset-react-app and generate its preset.
  const preset = presetReactApp(...args);

  // Replace the Flow preset with TypeScript.
  preset.presets = preset.presets.filter(
    p => !babelPluginTransformFlowDetectionHack(p)
  );
  preset.presets.push("@babel/preset-typescript");

  return preset;
};

export default removeFlowPreset;

// @babel/plugin-transform-flow-strip-types
const babelPluginTransformFlowDetectionHack = (preset: any): boolean => {
  /* tslint:disable-next-line:no-var-requires */
  const babel: any = require("@babel/core");

  try {
    const transformedCode = babel.transform(
      `function foo(one: any, two: number, three?): string {}`,
      {
        presets: [preset]
      }
    );
    return !transformedCode.code!.includes("string");
  } catch {
    return false;
  }
};

Can you try this? I haven't testing on old scripts version yet.

[detrohutt]

Sure, I'll give it a shot now. As an aside, I just had a thought.. Anyone using beta versions of software probably is already planning to upgrade as new betas become available (or at least when the stable version lands), so I'm not sure how important it is to remain backwards-compatible with older betas?

[detrohutt]

Would you mind transpiling out the import/export statements so I can throw it right into my node_modules folder?

[strothj]

"use strict";
var __importDefault = (this && this.__importDefault) || function (mod) {
    return (mod && mod.__esModule) ? mod : { "default": mod };
}
Object.defineProperty(exports, "__esModule", { value: true });
var babel_preset_react_app_1 = __importDefault(require("babel-preset-react-app"));
var removeFlowPreset = function () {
    var args = [];
    for (var _i = 0; _i < arguments.length; _i++) {
        args[_i] = arguments[_i];
    }
    // Pass along arguments to babel-preset-react-app and generate its preset.
    var preset = babel_preset_react_app_1.default.apply(void 0, args);
    // Replace the Flow preset with TypeScript.
    preset.presets = preset.presets.filter(function (p) { return !babelPluginTransformFlowDetectionHack(p); });
    preset.presets.push("@babel/preset-typescript");
    return preset;
};
exports.default = removeFlowPreset;
// @babel/plugin-transform-flow-strip-types
var babelPluginTransformFlowDetectionHack = function (preset) {
    /* tslint:disable-next-line:no-var-requires */
    var babel = require("@babel/core");
    try {
        var transformedCode = babel.transform("function foo(one: any, two: number, three?): string {}", {
            presets: [preset]
        });
        return !transformedCode.code.includes("string");
    }
    catch (_a) {
        return false;
    }
};

[detrohutt]

Thanks! Seems to work fine with my app (latest beta react-scripts). :)

[detrohutt]

Out of curiosity, could you explain why that works? ... Oh wait, is the code you're transforming flow annotations? I've never used flow so I just assumed it was TS, but if it's flow that makes more sense.

[strothj]

I looked up the documentation for the flow transform here:
https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-flow-strip-types

This is the same strategy you'd use when unit testing your own babel plugins and presets when developing.

I'm just passing along the preset to the babel transpiler and seeing if the tested preset strips out the flow type annotations.

[detrohutt]

Ok great, thanks for explaining! Do you have any idea when you may be able to get out an npm release with that code? Is there more testing that needs to be done? Anything I can do to help?

[strothj]

I'm going to spit out a release in a few minutes if all goes well. I'm testing a large project with the change to make sure the build time didn't balloon due to calling out to the babel compiler.

[detrohutt]

Excellent. You can close this PR whenever you like.

[strothj]

I've published version 2.1.0 with the changes. I was going to cherry-pick one of your commits but I think your branch was stale.

Anyways, thanks again for the help!