search

istanbuljs / babel-plugin-istanbul

A babel plugin that adds istanbul instrumentation to ES6 code
BSD 3-Clause "New" or "Revised" License
624 stars 73 forks source link

fix: Terminate NYC config loader in Electron #242

Closed inukshuk closed 3 years ago

inukshuk commented 4 years ago

In Electron the main process will not terminate automatically; this causes the plugin to hang during initialization. This commit calls process.exit explicitly after the config (or error message) has been sent back to the original process.

I added the isElectron check there to make sure everything stays the same in Node.js although it should be fine to just always exit by default.

inukshuk commented 4 years ago

Thanks!

I can totally understand if you can't officially support this on Electron, but, for what it's worth, the plugin works great: the main issue with Electron is that most of the time your tests run in multiple processes so the typical approach of calling istanbul does not work if you want to instrument code in everywhere. The typical approach to this is to instrument the code manually in each process and run nyc later on all the results; with this plugin this can be achieved with pretty much two lines of configuration and a hook to save the results, where previously we where using a full-blown custom mocha reporter.

coreyfarrell commented 3 years ago

FYI the next semver-major of babel-plugin-istanbul is going to be removing support for automatic loading of nyc config from outside nyc (a path you seem to be hitting). It is not a great solution to synchronously execute a process in order to load configuration. Instead the next version of babel-plugin-istanbul will generally expect you to provide options via babelrc options:

{
  "plugins": [
    ["babel-plugin-istanbul", {
      "include": ["lib/**"]
    }]
  ]
}

The only exception to this will be if babel-plugin-istanbul is run in a subprocess of nyc, in that case NYC_CONFIG can provide the default configuration.

Providing options to the current version of babelrc will prevent execution of the external configuration loader (I recommend doing so). This will also accelerate application startup by not spawning an external process.

Sorry it took me so long to look at this again. I'm not strictly opposed to merging this for 6.x but it will be for 6.x only. Let me know.

inukshuk commented 3 years ago

This sounds great, I'm all in favor of removing the automatic loading. Thanks for the heads-up!