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

Pass opts to BabelPluginIstanbul options #1

Closed
michaelhays opened this issue Nov 19, 2020 · 17 comments
Closed

Pass opts to BabelPluginIstanbul options #1

michaelhays opened this issue Nov 19, 2020 · 17 comments
Assignees
Labels
bug Something isn't working released

Comments

@michaelhays
Copy link

Thanks so much for this plugin, I would've been clueless without it!

Not sure if you're planning to continue maintaining this, but I had to tweak it a bit in order to instrument .vue files. Where plugins is defined (which is later passed to Babel's transformSync function), I had to include opts, like:

  const plugins = [[BabelPluginIstanbul, opts]]

Strangely, a couple of my .vue files were instrumented without this, but not all of them... 🤔

@michaelhays
Copy link
Author

michaelhays commented Nov 19, 2020

Spoke too soon; while this does instrument the .vue files, it does so on the JavaScript transpiled version rather than the original single-file component, so the line numbers don't match up. See here -- the green lines should correspond to the script at the bottom half of that file, rather than the template at the top.

I'm in a bit over my head with this, but I'll keep trying to figure this out and will post what I find here.

Update

When programmatically instrumenting code with babel-plugin-istanbul, you can pass an inputSourceMap so that the coverage is mapped back to the original source.

In Vite, the SFC compilation happens immediately before the transform in our custom plugin (see lines 124 and 115 here). After the SFC compilation, the map is stored to ctx.map.

Unfortunately, that map is not provided in the context to our transform function here. If this were instead:

const result = await t.transform({
  ...transformContext,
  code,
  map: ctx.map,  // added
})

then we could simply pass that to the babel plugin, like:

plugins: [[BabelPluginIstanbul, { ...opts, inputSourceMap: ctx.map }]],

Indeed, this works for me locally when I add that to the file in my node_modules/. But I don't know if there is any other way to provide that context.

@iFaxity
Copy link
Owner

iFaxity commented Nov 21, 2020

Hi, yes i intend to keep maintaining this as i depend on it in other projects i have, however i have only tested it with JS and not any custom transforms like JSX or Vue's SFC. I only have time on the weekends to go on GitHub and to spend time on my projects, which is why i only saw the issue now.

I'll look into the issue and get back when i have a solution, i'll fork your project to be able to reproduce the bug. However as you said the issue of slotting in the custom sourceMap should maybe be handled on Vite's end.

Great detective work btw 😀👍, i'll look into this tomorrow and probably we can work out a solution that works best for Vue.

@iFaxity iFaxity added the bug Something isn't working label Nov 21, 2020
@iFaxity iFaxity self-assigned this Nov 21, 2020
@michaelhays
Copy link
Author

Hey appreciate the response! Yeah no worries of course, I checked in in case you had any quick ideas, but no need to dig too hard. I may actually make a PR to Vite to see if they'll pass the map to transform, since that solves the issue for my use case

@iFaxity
Copy link
Owner

iFaxity commented Jan 8, 2021

@michaelhays Hey, it seems that there is now a way to get the source map in the Vite 2.0 beta, it seems that there is an added method called getCombinedSourcemap that generates the current source map from all the previous transforms.

I'll adjust the plugin and should have a patch up in the weekend to add the feature. Then maybe you could verify if it works in your project (if you haven't already fixed it some other way).

@michaelhays
Copy link
Author

Hey thanks so much for the help. I'd tried some things with the map in the Vite 2.0 beta, but hadn't seen getCombinedSourcemap -- definitely seems like that's the right approach.

I ended up going with this:

import istanbulInstrument from 'istanbul-lib-instrument'
import { createFilter } from '@rollup/pluginutils'

export default function istanbul(options) {
  const filter = createFilter(options.include, options.exclude)

  return {
    name: 'istanbul',

    transform(src, id) {
      if (
        process.env.NODE_ENV == 'production' ||
        id.startsWith('/@modules/') ||
        id.includes('node_modules') ||
        !filter(id)
      ) {
        return
      }

      let scriptSrc = src
      let scriptStartIndex
      let scriptEndIndex

      if (id.endsWith('.vue')) {
        scriptStartIndex = src.indexOf('<script>')
        scriptEndIndex = src.indexOf('</script>')

        if (scriptStartIndex === -1 || scriptEndIndex === -1) {
          return
        }

        scriptStartIndex += '<script>'.length

        const numNewlines = (src.slice(0, scriptStartIndex).match(/\n/g) || [])
          .length
        scriptSrc =
          '\n'.repeat(numNewlines) + src.slice(scriptStartIndex, scriptEndIndex)
      }

      const instrumenter = new istanbulInstrument.createInstrumenter({
        esModules: true,
        compact: true,
        produceSourceMap: true,
        autoWrap: true,
        preserveComments: true,
      })

      let code = instrumenter.instrumentSync(scriptSrc, id)

      if (id.endsWith('.vue')) {
        code =
          src.slice(0, scriptStartIndex) +
          '\n' +
          code +
          '\n' +
          src.slice(scriptEndIndex)
      }

      return code
    },
  }
}

This plugin runs before the Vue plugin. Basically, for .vue files, it takes whatever content is between the <script> tags, instruments only that, and inserts it back into the original source. Super inelegant, but it works 😆

So don't feel any obligation to patch this, unless you'd like to offer the functionality as part of the plugin. I know it's basically just you and me right now, but hopefully more people start using this as Vite matures. If you do add it, I'm happy to test out any changes you make with my codebase.

@iFaxity
Copy link
Owner

iFaxity commented Jan 8, 2021

No I'll fix it because I need to update the package anyways to fit the new plugin spec. I'll work on a patch this weekend. I'll report back when it's done so you can check if it works.

@iFaxity
Copy link
Owner

iFaxity commented Jan 10, 2021

@michaelhays Hey again, version 2.x of the plugin is now compatible with Vite 2.0, also added the aforementioned patch with the source maps. Could you please verify if it seems to work well with the coverage in your project?

@michaelhays
Copy link
Author

A few issues I'm having:

  1. Looks like Babel's transformSync expects inputSourceMap to be a plain object, while this.getCombinedSourcemap() yields a SourceMap (I think? It outputs SourceMap { ... } to the console).

Error I was getting:

4:22:39 PM [vite] Internal server error: don't know how to turn this value into a node
  Plugin: vite:istanbul
  File: /path/to/project/web/src/main.js
  Error: don't know how to turn this value into a node
      at valueToNode (/path/to/project/web/node_modules/@babel/types/lib/converters/valueToNode.js:90:9)
      at Object.valueToNode (/path/to/project/web/node_modules/@babel/types/lib/converters/valueToNode.js:84:58)
      at Object.exit (/path/to/project/web/node_modules/istanbul-lib-instrument/dist/visitor.js:641:30)
      at PluginPass.exit (/path/to/project/web/node_modules/babel-plugin-istanbul/lib/index.js:158:38)
      at newFn (/path/to/project/web/node_modules/@babel/traverse/lib/visitors.js:175:21)
      at NodePath._call (/path/to/project/web/node_modules/@babel/traverse/lib/path/context.js:55:20)
      at NodePath.call (/path/to/project/web/node_modules/@babel/traverse/lib/path/context.js:42:17)
      at NodePath.visit (/path/to/project/web/node_modules/@babel/traverse/lib/path/context.js:101:8)
      at TraversalContext.visitQueue (/path/to/project/web/node_modules/@babel/traverse/lib/context.js:115:16)
      at TraversalContext.visitSingle (/path/to/project/web/node_modules/@babel/traverse/lib/context.js:84:19)

I fixed this by changing this line to inputSourceMap: JSON.parse(JSON.stringify(this.getCombinedSourcemap())),.


  1. I believe this line here needs to be const plugins = [[BabelPluginIstanbul, opts]];, or the transformSync will ignore .vue files by default.

  1. There's one other bug that I was coming across when trying to get this working in Vite 2 a week ago, where the .vue file doesn't have a <script> section, and thus the Vue plugin outputs a map of just { mappings: '' }. You can see that here. When Vite tries to combine all of the plugin map outputs at the end, it throws this error:
4:45:15 PM [vite] Internal server error: "version" is a required argument.
  Plugin: vite:istanbul
  File: /path/to/project/web/src/App.vue
  Error: "version" is a required argument.
      at Object.getArg (/path/to/project/web/node_modules/@babel/core/node_modules/source-map/lib/util.js:24:11)
      at new BasicSourceMapConsumer (/path/to/project/web/node_modules/@babel/core/node_modules/source-map/lib/source-map-consumer.js:288:22)
      at new SourceMapConsumer (/path/to/project/web/node_modules/@babel/core/node_modules/source-map/lib/source-map-consumer.js:22:7)
      at buildMappingData (/path/to/project/web/node_modules/@babel/core/lib/transformation/file/merge-map.js:148:20)
      at mergeSourceMap (/path/to/project/web/node_modules/@babel/core/lib/transformation/file/merge-map.js:21:17)
      at generateCode (/path/to/project/web/node_modules/@babel/core/lib/transformation/file/generate.js:74:39)
      at runSync (/path/to/project/web/node_modules/@babel/core/lib/transformation/index.js:50:51)
      at Object.transformSync (/path/to/project/web/node_modules/@babel/core/lib/transform.js:43:38)
      at TransformContext.<anonymous> (/path/to/project/web/node_modules/vite-plugin-istanbul/dist/index.js:46:42)
      at Object.transform (/path/to/project/web/node_modules/vite/dist/node/chunks/dep-0071bc7e.js:52598:53)

I have to change that return to this in order to get this working:

  return {
    code: output.join("\n"),
    map: resolvedMap || {
      mappings: "",
      version: 3,
      sources: []
    }
  };

Not sure if this is a bug with Vite, or if there's something else I'm missing.


Unfortunately, even when I make all of these changes in node_modules/, the code coverage still doesn't map back to the .vue files correctly 😅. I think it's mapping onto the transpiled JavaScript, but not the original <script> in the .vue file. I had the same problem when I tried to get this working last week, before giving up and going with the solution I described in this comment. * Sigh *... oh well

@iFaxity
Copy link
Owner

iFaxity commented Jan 17, 2021

Weird, i'll get back to this in a week or so and see if i can add the fix you provided earlier, maybe do the fix only if the id contains a .vue extension.

Thanks for describing the other bugs with the SourceMap too.

@sdvinfo
Copy link

sdvinfo commented Mar 24, 2021

It looks like this plugin is still not working. I tried to get Cypress to work with it for three days, but to no avail.
As a result, the plugin from @michaelhays completely solved my problem for today.
Thank @michaelhays!
@iFaxity we are waiting for the development of the project. ;)

@iFaxity
Copy link
Owner

iFaxity commented Mar 24, 2021

Thanks for reminding me, I've taken an unplanned break from coding in my freetime. But I will look into @michaelhays solution again and see if I can integrate something based on that.

I remember not doing it before because I thought there was a better solution. But solution is in fact better than no solution after all 😊.

However I will push out an update today or tomorrow.

@michaelhays
Copy link
Author

Glad it was helpful @sdvinfo! :)

A couple of other things that I've done since then that might help: the plugin doesn't generate a source map, so the files loaded in the browser will contain the instrumented code. This can make debugging difficult, so I added an environment variable to only instrument the code when running Cypress tests.

In vite.config.js:

import istanbul from './vite-plugin-istanbul'
import vue from '@vitejs/plugin-vue'

/**
 * @type {import('vite').UserConfig}
 */
const config = {
  plugins: [vue()],
}

if (process.env.VITE_INSTRUMENT_CODE === 'TRUE') {
  config.plugins = [
    istanbul({
      include: ['src/**/*.js', 'src/**/*.vue'],
    }),
    ...config.plugins,
  ]
}

export default config

Similarly, in the Cypress plugin config:

/**
 * @type {Cypress.PluginConfig}
 */
module.exports = async (on, config) => {
  if (process.env.VITE_INSTRUMENT_CODE === 'TRUE') {
    require('@cypress/code-coverage/task')(on, config)
  }

  return config
}

@michaelhays
Copy link
Author

Yeah @iFaxity my solution was definitely a bit hacky -- I know very little about instrumenting or source maps, so I went with the most "direct" way to instrument the <script> section for Vue SFCs. But I'm fairly certain there's a better solution.

@iFaxity
Copy link
Owner

iFaxity commented Mar 25, 2021

Yeah me neither, ill add the ENV variable too so it doesn't break your code. 👍

@iFaxity
Copy link
Owner

iFaxity commented Mar 25, 2021

Version 2.1.0 should fix this, I ended up using the env variable VITE_COVERAGE instead. Then you can also use the new cypress option along with requireEnv to explicitly require the CYPRESS_COVERAGE variable to be set in order to instrument the code.

I haven't tested it thoroughly though, only tested with simple Vite demos of React and Vue. Can you verify that it is working?

@elevatebart
Copy link
Contributor

In the PR above, I managed to have coverage with @cypress/vite-edv-server by making sure the instrumentation transform is done last, on simpler JS. I am not even sure that the babel options are necessary. Nor the special treatment of Vue SFC files.

Tell me what you think.

@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 2.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

No branches or pull requests

4 participants