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

Add sourceMapCallback option to enable advanced mapping use cases #69

Merged
merged 2 commits into from Mar 28, 2018

Conversation

lephyrus
Copy link
Contributor

Motivation
I am trying to do a Rollup build that transpiles my Typescript sources and specs for a Karma run, while also instrumenting the sources in a way that maps back to the original Typescript, without writing intermediate files to disk. I also want the final Rollup bundle to have correct source maps for in-browser debugging.

Problem
This plugin provides Rollup with the maps for the transformation from Typescript to Javascript, which correctly maps the resulting bundle to the original sources. When instrumenting the code with Istanbul as part of the process, the coverage info will not correctly apply to the original typescript, however. One approach is to use remap-istanbul (e.g. via karma-remap-coverage), but for that you need the source maps for the Typescript > Javascript transformation. These are not accessible fior a Rollup plugin, unless written to disk, which is not what this plugin does (for good reason). If I set inlineSourceMap to true in the Typescript compiler options, the Istanbul instrumenter correctly sees this as an "input sourcemap" and the coverage maps back to the Typescript sources perfectly. With such a configuration, rollup-plugin-typescript2 on the other hand does not "see" the sourcemap and does not pass it back to Rollup, which leads to incorrect mappings for the generated bunde. The other way to solve this is to "manually" supply the Istanbul instrumenter with an input source map. This is what I got working.

Solution
With the help of the modification proposed in this PR and a custom instrumenter plugin, I got the coverage report to match the Typescript sources while also having the correct source map for the bundle produces by Rollup. 🎉

// karma.conf.js
// ...
  config.set({
    // ...
    files: [
      // ...
      { pattern: 'specs.ts', watched: false }
    ],
    preprocessors: { 'specs.ts': ['rollup'] },
    reporters: ['coverage-istanbul'],
    mime: { 'text/x-typescript': ['ts','tsx'] },
};
// rollup.config.js
// ...
let typescriptSourceMaps = new Map();

module.exports = {
  plugins: [
    resolve(),
    typescript(
      tsconfigOverride: { include: 'specs.ts' },
      clean: true,
      sourceMapCallback: (id, map) => typescriptSourceMaps.set(id, map)
    }),
    commonjs(),
    instrument({
      include: 'src/**/*.ts',
      exclude: '**/*.spec.ts',
      getInputSourceMap: (id) => typescriptSourceMaps.get(id)
    }),
  ],
  output: {
    format: 'iife',
    sourcemap: 'inline'
  },
};
// rollup-plugin-instrument.js
import { createInstrumenter } from 'istanbul-lib-instrument';
import { createFilter } from 'rollup-pluginutils';

export default function instrument({ include, exclude, getInputSourceMap, debug } = {}) {
  const filter = createFilter(include, exclude);
  const instrumenter = createInstrumenter({
    esModules: true,
    produceSourceMap: true
  });
  getInputSourceMap = getInputSourceMap || (() => undefined);

  return {
    name: 'instrument',

    transform(code, id) {
      if (!filter(id)) return;

      let inputMap;
      try {
        inputMap = JSON.parse(getInputSourceMap(id));
      } catch (error) {
        inputMap = undefined;
      }

      const instrumentedCode = instrumenter.instrumentSync(code, id, inputMap);
      const outputMap = instrumenter.lastSourceMap();

      return { code: instrumentedCode, map: outputMap };
    }
  };
}

Notes

  • Sorry, it's a lot of explanation for a very small change. But man, this stuff was driving me crazy.
  • I think omitting the clean: true option leads to problems with this approach, not sure why.
  • Possibly related issue: Problems running with typescript artberri/rollup-plugin-istanbul#13,
  • By the way, I could not figure out how to limit all these changes to the dist files. I think they are caused by new versions of dependencies. I don't understand how package.lock.json is supposed to work. I really don't. 🤷‍♂️

@ezolenko
Copy link
Owner

Don't worry about changes in dist, I'll rebuild it anyway.

Looks good aside from breaking on cache (see comment inline)

src/index.ts Outdated Show resolved Hide resolved
@ezolenko ezolenko self-assigned this Mar 23, 2018
Note that the reason for passing a JSON string rather than an object
reference is that Rollup mutates the object, leading to unpredictable
behaviour.
@lephyrus
Copy link
Contributor Author

I still don't fully get how caching works, but it looks to me like the sourcemap JSON will still only be parsed once (unless a file changes) with the how it's done now.

@ezolenko ezolenko merged commit ffb2c0d into ezolenko:master Mar 28, 2018
@ezolenko
Copy link
Owner

Ok, I fixed some formatting and I think there was a bug in the way I was returning maps from cache, but rollup might have been covering for it.

@lephyrus Could you check if current version works for you both clean and from cache?

@lephyrus
Copy link
Contributor Author

@ezolenko Yep, seems to work perfectly either way. Thanks for incorporating this feature so quickly!

@lephyrus
Copy link
Contributor Author

@ezolenko Can I ask if you're planning to do a release soon?

@ezolenko
Copy link
Owner

Yep, most likely on the weekend sometime. I'm waiting on confirmation for one other issue, but that's not critical.

@agilgur5 agilgur5 changed the title Add 'sourceMapCallback' option to enable advanced mapping use cases Add sourceMapCallback option to enable advanced mapping use cases Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants