Automatic Babel Transformation problematic

[DrewML]

Hey Orta!

Thanks for all your hard work on Danger. Fantastic tool 😃.

I'm running into an issue with the automatic Babel transformation that I wanted to discuss with you and any other collaborators before submitting a Pull Request.

My project is a pretty standard webpack/babel compiled front-end application. I'm running Danger on node 8.x, so I really do not need Babel transformation of my dangerfile.

Right now, because of my babel-preset-env config for builds targeting web browsers, template literals in my dangerfile are being compiled to ES5. This would be fine, except my .babelrc is setup to use transform-runtime. This is problematic because Babel 6 has a bug where transform-runtime injects import declarations rather than require calls. After this happens, Danger fails on calls to node's module._compile.

I can work around this issue by moving everything in my Babel config to use the env feature temporarily, and have no "default" babel config. However, I'm concerned that the automatic Babel transformation in Danger without an opt-out is a bit heavy-handed.

Would you be willing to accept a PR that disables Babel/TypeScript transformation?

[orta]

Yep, a CLI flag makes sense for that 👍

[sebinsua]

I've run into problems related to this a few times, too.

The transpiler.ts file is quite problematic for me. I use Babel 7 TypeScript but it is not detected because I'm using the scoped version of the Babel packages -- what ends up happening is that transform-flow-strip-types gets picked up due to it being installed within my monorepo by Storybook's babel-preset-react (via babel-preset-flow), and finally my babel.config.js file gets loaded with its actual @babel/preset-typescript and obviously this is not a valid configuration so it fails.

I definitely think this is something we should be able to switch off. Perhaps it'd also be possible to change the defaults here?

@DrewML Are you still going to submit a PR request for this?

[orta]

Couldn't the in-built transpiler be smarter to handle that case? I'd rather not have escape clauses if we can just cover cases right

[sebinsua]

Yes, that might work, too. When I get back to work next week, I will try to amend the file and work out what works. :)

[sebinsua]

I started trying to improve the built-in transpiler. However, because of the .babelrc file at the root containing the plugin-transform-flow-strip-types, it wasn't possible to handle using hasBabelTypeScript over hasNativeTypeScript without getting the error about the Flow and TypeScript plugins being loaded at the same time.

I think it should prefer Babel 7 Typescript over normal TypeScript because if you have the former you probably also have the latter for later type-checking. And additionally you are probably doing your builds with Babel 7 (does this judgement seem correct to others?).

Therefore, I removed the Flow plugin from the root .babelrc and the tests passed.

However, I then got an error on the prepush hook about "Missing class properties transform.". I added this to the .babelrc and then got a debugModule is not a function from within source/debug.ts:15:11.

I think this is because I'm now causing this to all be run with Babel TypeScript and your code wasn't written with esModuleInterop, etc?

I think it's a decent idea to prefer Babel 7 TypeScript over Native TypeScript when it is available, but do you mind if I rewrite all of your imports? 😛

[orta]

The babel stuff is in there for testing, I think, so that could just move out of the root folder?

[sebinsua]

Hm, I didn't know where to move it?

I did what I suggested though and switched tsconfig.json#esModuleInterop to true and then altered a bunch of imports. The tests pass and the push checks are no longer failing. I might take the package produced and test it on my work machine to see whether this does what I expect, but I think it does...

Edit: I could be completely off-base here -- do you want the behavior to be to use Babel 7 TypeScript over TypeScript or the other way around?

Edit 2: I did something speculative, by trying to remove the flow plugin from the plugins array if you have the typescript plugin and vice versa. That doesn't seem to work. I think because it's not loaded the babelrc options which contains this fully yet... Fixed: but you need to be on the latest version of Babel 7.

[orta]

Cool, so: I never intended for danger-js to be transpiled with babel (I want the d.ts files to ship with the dependency) - but I did want it around specifically for writing integration tests that use babel to transpile a dangerfile (so that I was certain that it worked for people, as we always use typescript dangerfile)

Having it work that was is 👍 to me, but not a goal - I just want to verify that a client can do it. Maybe babel doesn't even need to be in danger-js's package.json at all - the test that uses CRA will verify that it transpires correctly

[sebinsua]

Maybe babel doesn't even need to be in danger-js's package.json at all

I could do that.

the test that uses CRA will verify that it transpires correctly

Which test is this?

Cool, so: I never intended for danger-js to be transpiled with babel

FYI, after I promoted Babel 7 TypeScript over plain TypeScript, it was this stuff which caused me to have to change imports (since it meant that the non-transpiled source was being used). But I guess if you want, I can remove all of the Babel packages from devDependencies so that it falls back to plain-old TypeScript.

[orta]

That test lives outside of jest, https://github.com/danger/danger-js/blob/master/.travis.yml#L60-L83

Yeah, I think removing it should be fine 👍

[sebinsua]

I can't remove Babel entirely because it causes certain unit tests to fail.

[orta]

I'm happy to have those deleted, so long as we have the async transformation validated in the fixture tests which I think is definitely happening.

[jakubzitny]

Hi, how did you resolve this issue? Was it addressed in a PR and merged or just worked around?

I am trying to improve the danger-plugin-flow and using it from my repo with babel6 and flow but the plugin import fails on the transpiler.js.

Unknown plugin "babel-plugin-transform-flow-strip-types" specified in "base" at 0, attempted to resolve relative to "/danger-plugin-flow/dist"
ReferenceError: Unknown plugin "babel-plugin-transform-flow-strip-types" specified in "base" at 0, attempted to resolve relative to "/danger-plugin-flow/dist"
    at /app/node_modules/babel-core/lib/transformation/file/options/option-manager.js:180:17
    at Array.map (<anonymous>)
    at Function.normalisePlugins (/app/node_modules/babel-core/lib/transformation/file/options/option-manager.js:158:20)
    at OptionManager.mergeOptions (/app/node_modules/babel-core/lib/transformation/file/options/option-manager.js:234:36)
    at OptionManager.init (/app/node_modules/babel-core/lib/transformation/file/options/option-manager.js:368:12)
    at File.initOptions (/app/node_modules/babel-core/lib/transformation/file/index.js:212:65)
    at new File (/app/node_modules/babel-core/lib/transformation/file/index.js:135:24)
    at Pipeline.transform (/app/node_modules/babel-core/lib/transformation/pipeline.js:46:16)
    at Object.exports.babelify (/app/node_modules/danger/distribution/runner/runners/utils/transpiler.js:132:18)
    at Object.exports.default (/app/node_modules/danger/distribution/runner/runners/utils/transpiler.js:152:26)

Adding babel-plugin-transform-flow-strip-types to the danger-plugin-flow solves this but I don't think that's a good solution at all.

EDIT: Ok, so I found DANGER_DISABLE_TRANSPILATION in the code, which does disable transpilation. But then I can't write ES6 dangerfile which is not good either.

EDIT2: Okay, I found a clear explanation and have a proposal for improvement. The transpilation transpiles a Dangerfile from a project that uses danger, but it follows the imports in dangerfile and tries to transpile them as well. While this is correct for local imports in Dangerfile, it should definitely not follow library imports (and especially danger plugins) and try to compile them. Would you be open for such a PR?

EDIT3: Quick fix for any projects having problems with babel+flow in particular is adding a symlink from parent project to the danger plugin.

ln -s `pwd`/node_modules/babel-plugin-transform-flow-strip-types node_modules/danger-plugin-flow/node_modules/babel-plugin-transform-flow-strip-types

[mgol]

+9001 for allowing to disable transpilation. The Babel transpilation messes up lines & columns so when I see an error I don't know where it really comes from. With new Node.js there're not many reasons to transpile from newer JavaScript as Node.js handles these constructs just fine. JavaScript transpilation not only introduces some overhead but also causes problems while giving virtually no advantages over just using new Node.js...

[orta]

Cool, yeah sure, you're welcome to add an option, or an env var etc 👍

I use typescript for all my dangerfiles, so it's not a pain for me

[mgol]

I started looking into it and I noticed there's already an env var: DANGER_DISABLE_TRANSPILATION needs to be set to "true". That solves it for me.

[orta]

Nice, I think this is worth adding to https://github.com/danger/danger-js/edit/master/docs/tutorials/transpilation.html.md - as that's likely a first port of call for folks looking into the problem

[bj-mcduck]

Just to chime in, the default example of setting up an action with the following gets the same Error: Cannot find module '@babel/plugin-transform-flow-strip-types' from '/github/workspace' error. Sounds like it's the same issue?

name: Danger checks

on: pull_request

jobs:
  build:
    runs-on: ubuntu-latest

    steps:
    - uses: actions/checkout@v2
    - name: Danger
      uses: danger/[email protected]
      env: 
        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

[pietmichal]

@brandonjmckay Setting DANGER_DISABLE_TRANSPILATION="true" env var fixed it for me.

[dereckquock]

got this working by adding @babel/plugin-transform-flow-strip-types to plugins in babel config

and I'm not using the github action:

 - name: Danger JS
   run: npx danger ci
   env:
      GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}