Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

Upgrading from babel-istanbul-loader, build fails with JSX #44

Closed
ssylvia opened this issue Feb 14, 2017 · 25 comments
Closed

Upgrading from babel-istanbul-loader, build fails with JSX #44

ssylvia opened this issue Feb 14, 2017 · 25 comments

Comments

@ssylvia
Copy link

ssylvia commented Feb 14, 2017

I saw that babel-istanbul-loader was deprecated and was pointed to this library as the upgrade. My webpack now fails to build if there is JSX syntax in code.

Does istanbul-instrumenter-loader support react JSX? Do I need any other changes?

Previous config from babel-istanbul-loader:

module: {
      rules: [
        {
          include: [
            path.resolve(__dirname, './src'),
          ],
          test: /\.(js|jsx)$/,
          use: [
            {
              loader: 'babel-loader',
              options: {
                cacheDirectory: true,
              },
            },
          ],
        },
        {
          enforce: 'pre',
          exclude: /\.test\.js|jsx$/,
          include: [
            path.resolve(__dirname, './src'),
          ],
          test: /\.(js|jsx)$/,
          use: [
            {
              loader: 'babel-istanbul-loader',
              options: {
                cacheDirectory: true,
              },
            },
          ],
        },

new config with istanbul-instrumenter-loader:

module: {
      rules: [
        {
          include: [
            path.resolve(__dirname, './src'),
          ],
          test: /\.(js|jsx)$/,
          use: [
            {
              loader: 'babel-loader',
              options: {
                cacheDirectory: true,
              },
            },
          ],
        },
        {
          exclude: /\.test\.js|jsx$/,
          include: [
            path.resolve(__dirname, './src'),
          ],
          test: /\.(js|jsx)$/,
          use: [
            {
              loader: 'istanbul-instrumenter-loader',
              options: {
                esModules: true,
              },
            },
          ],
        },

.babelrc

{
  "plugins": ["dynamic-import-webpack"],
  "presets": [
    "latest",
    "react",
    "stage-3"
  ],
  "env": {
    "test": {
      "plugins": ["istanbul"]
    }
  }
}
@deepsweet
Copy link
Collaborator

really weird, it should use the same Babel with .babelrc under the hood... could you please show me the exactly error message?

@ssylvia
Copy link
Author

ssylvia commented Feb 15, 2017

@deepsweet Here's the error I get:

webpack: wait until bundle finished:

ERROR in ./src/app/index.js
Module build failed: SyntaxError: Unexpected token (11:2)
    at Parser.pp$5.raise (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:4333:13)
    at Parser.pp.unexpected (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:1705:8)
    at Parser.pp$3.parseExprAtom (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:3670:12)
    at Parser.pp$3.parseExprSubscripts (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:3414:19)
    at Parser.pp$3.parseMaybeUnary (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:3394:19)
    at Parser.pp$3.parseExprOps (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:3324:19)
    at Parser.pp$3.parseMaybeConditional (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:3301:19)
    at Parser.pp$3.parseMaybeAssign (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:3264:19)
    at Parser.pp$3.parseExprListItem (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:4190:16)
    at Parser.pp$3.parseCallExpressionArguments (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:3493:20)
    at Parser.pp$3.parseSubscripts (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:3453:31)
    at Parser.pp$3.parseExprSubscripts (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:3424:15)
    at Parser.pp$3.parseMaybeUnary (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:3394:19)
    at Parser.pp$3.parseExprOps (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:3324:19)
    at Parser.pp$3.parseMaybeConditional (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:3301:19)
    at Parser.pp$3.parseMaybeAssign (/Users/myUserName/appRepo/node_modules/babylon/lib/index.js:3264:19)
webpack: Failed to compile.

This is my ./src/app/index.js file:

import 'babel-polyfill';
import App from './app.jsx';
import AppStore from './store/AppStore';
import {Provider} from 'react-redux';
import React from 'react';
import {render} from 'react-dom';

const appStore = new AppStore();

render(
  <Provider store={appStore}>
    <App />
  </Provider>, document.getElementById('app')
);

@bencripps
Copy link

Having this exact issue as well. I've got the "react" preset listed in my .bablrc and I've got the following loader attached:

{
        test: /\.js$|\.jsx$/,
        enforce: 'pre',
        exclude: /node_modules|test/,
        loader: 'istanbul-instrumenter-loader',
        query: {
            esModules: true,
            presets: ['react']
        }
    }

@bencripps
Copy link

Any idea on this one? I'd like to upgrade to webpack2, but I don't wanna lose code coverage.

@deepsweet
Copy link
Collaborator

looks like there should additional plugins: [ 'jsx', 'flow' ] option herebabelrc has nothing to do with Babylon.

@bencripps
Copy link

bencripps commented Feb 19, 2017

Since babylon doesnt expect plugins or presets, doesn't it seem like the instrumenter is expecting already-transpiled code?

How did this work before with JSX/ES6? Was the code already transpiled before it got to the instrumenter?

@deepsweet
Copy link
Collaborator

this one didn't work before with JSX/ES6 :) that's why we had babel-istanbul-loader and isparta-loader.

@bencripps
Copy link

Hmm, what should we do now since both those packages have been deprecated?

@deepsweet
Copy link
Collaborator

they are deprecated because now we finally have "an official" solutiuon, new Istanbul integrated with Babel, but code coverage is a forever pita ¯\_(ツ)_/¯ try https://github.com/istanbuljs/babel-plugin-istanbul

I personally have no idea how do they want to transpile it directly with Babylon and allow you to use any non-default transforms like JSX, Flow, etc at the same time.

@bencripps
Copy link

I'll give that a shot, thanks!

@unimonkiez
Copy link

unimonkiez commented Mar 1, 2017

@bencripps How did it go?

@bencripps
Copy link

bencripps commented Mar 1, 2017

Success! Here is the changeset.

The issue was that, previously, I had used the istanbul loader in the pre stage -- rather than transpile the code before it gets to the pre stage, I am now instrumenting the code after the loaders take effect.

Here's a post about the upgrade.

@Axbon
Copy link

Axbon commented Mar 9, 2017

I have tried this and this does not work well for us. This used to work with webpack 1 and the old "loader: 'babel-istanbul". We used this in the "preloader" stage, which correctly instrumented the es6/jsx code and coverage was correct. After upgrading to webpack 2 and now using the istanbul-instrumenter-loader it only works in the "post" stage, which to me seems incorrect. It's not desired to get coverage on transpiled code, since this code is not the original, its created by babel. If used in the "pre" stage it gets the error Unexpected Token as if it doesnt support JSX - but how did babel-instanbul support that? Whats the major difference here?

I am also curious why it seems to be working for bencripps, I mean... is your coverage really correct, on the transpiled code? Do you get undefined line numbers in the report?

@deepsweet
Copy link
Collaborator

#48

@Axbon
Copy link

Axbon commented Mar 10, 2017

I solved the problem regarding jsx support in pre-stage in istanbul-lib-instrument, will fork or try to get a pr through there.

@Axbon Axbon mentioned this issue Mar 10, 2017
@glenpike
Copy link

Sorry to butt in on this tthread - my question is for @Axbon
I tried using your fork instead with the following setup in package.json

"istanbul-instrumenter-loader": "git://github.com/Axbon/istanbul-instrumenter-loader.git#87f7ede",

But npm fails with:

npm ERR! git rev-list -n1 1.4.2: fatal: ambiguous argument '1.4.2': unknown revision or path not in the working tree.

I might be wrong, but I think you might need to point your temp dependency url for istanbul-lib-instrument to your commit #c50e2cb rather than 1.4.2? e.g.

"istanbul-lib-instrument": "git://github.com/Axbon/istanbul-lib-instrument.git#c50e2cb",

@Axbon
Copy link

Axbon commented Apr 3, 2017

Ah true, anyway that fork was a temporary thing just to get JSX support added. I am happy to report that it has now landed in istanbul-lib-instrument@1.7.0 , perhaps @deepsweet could update the dependency so that jsx works, otherwise happy to send pr :)

@kulakshay
Copy link

kulakshay commented Apr 5, 2017

Hey @Axbon, I tried using istanbul-lib-instrument@1.7.0 still not working for jsx code.
I tried in both pre and post modes.

@Axbon
Copy link

Axbon commented Apr 6, 2017

@kulakshay seems like they made a change and moved the istanbul-lib-instrument, it now says: deprecated, instead use https://github.com/istanbuljs/istanbuljs so I suppose the dependency has to be updated there, and I verified that the jsx support is indeed merged into that codebase, not the other one however. Sorry for the confusion! The code can be found here: https://github.com/istanbuljs/istanbuljs/tree/master/packages/istanbul-lib-instrument and I guess this repo has to be updated, so that the instrumenter is imported from istanbuljs instead.

@kulakshay
Copy link

Thanks @Axbon!
But I am little confused here, do you mean that I use istanbuljs instead of istanbul-lib-instrument ?
Should I just add dependency for istanbuljs in my copy of istanbul-instrumenter-loader and remove istanbul-lib-instrument ?

@Axbon
Copy link

Axbon commented Apr 11, 2017

That is a good question, I suppose the guys behind istanbuljs have to re-publish to npm so that the package comes from their "monorepo" since it was migrated there. There appears to be no "istanbuljs" on npm where you would get access to all packages there :( I'll try to find out

@kulakshay
Copy link

Thanks @Axbon, my current status is I can generate report in 'post' mode, but 'pre' mode is still not working.
So there is huge gap between previous coverage results (using isparta in pre mode) and current implementation.

@unimonkiez
Copy link

unimonkiez commented Apr 26, 2017

My coverage dropped about 10% while upgrading from isparta-loader (pre mode) to istanbul-instrumenter-loader (post mode).

Unable to change to pre without though..

EDIT:
Removed the 'enforce' on 'istanbul-instrumenter-loader' and moved it to be first in the loaders array (otherwise it wouldn't work), that made my coverage increase a lot but still 2% below isparta-loader.
Checking a sample file and still shows I have a problem (2/10 coverage produces 16.67%)

EDIT 2:
Managed to get it to work, all I needed to do is to upgrade to babel-loader v7.0.0 (which was released just several days ago), but still leave the loader to be first in the array.

You can check my repo with the implementation (webpack line).

documentation should be updated to state the solution.

@kulakshay
Copy link

@unimonkiez I'll definitely give it a shot, thanks !!!

@mattlewis92
Copy link
Member

The recommended path to getting babel + istanbul instrumentation to work is to use the babel plugin to instrument your code, rather than this loader. Just remove this loader and add that babel plugin to your babel config when testing and everything should work great! 😄

stvnjacobs added a commit to stvnjacobs/manager that referenced this issue Jul 12, 2017
This brings manager and docs up to the latest version of webpack 2.6
release. It will allow for future improvements that are only available
in webpack 2+.

Changes include:

- Updated syntax for webpack loaders. [link](https://webpack.js.org/guides/migrating/#module-loaders-is-now-module-rules)
- Use the babel-plugin-istanbul to instrument code for tests,
  as per recommendation by the webpack-contrib team. [link](webpack-contrib/istanbul-instrumenter-loader#44 (comment))
- Use `cross-env` for all npm scripts to improve compatibility.
eatonphil pushed a commit to linode/manager that referenced this issue Jul 13, 2017
* upgrade webpack to ^2.6.0

This brings manager and docs up to the latest version of webpack 2.6
release. It will allow for future improvements that are only available
in webpack 2+.

Changes include:

- Updated syntax for webpack loaders. [link](https://webpack.js.org/guides/migrating/#module-loaders-is-now-module-rules)
- Use the babel-plugin-istanbul to instrument code for tests,
  as per recommendation by the webpack-contrib team. [link](webpack-contrib/istanbul-instrumenter-loader#44 (comment))
- Use `cross-env` for all npm scripts to improve compatibility.

* revert to npm test scripts without cross-env

* update missed loader config to use
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants