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

Added option to transform the paths within a sourcemap. #2430

Merged
merged 4 commits into from
Sep 13, 2018

Conversation

sebinsua
Copy link
Contributor

@sebinsua sebinsua commented Aug 28, 2018

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevent issue numbers:

Description

@pgn-vole wrote:
It doesn't seem possible at the moment to customize the path the sources within the sourcemap.

I added a OutputOption#sourcemapPathTransform function which can be used to transform the source paths outputted into the sourcemaps. This helps in situations in which multiple rollup generated libraries sourcemaps are being consumed in a browser and you do not want all package's files to be grouped within the same src/ directory.

I do not think it's necessary to supply a base directory as was mentioned in the original GitHub issue. A transformation function can also handle making the path relative to a base directory. If you're able to pass a transform function in through a rollup.config.js you will also be able to pass a base path into your transformation function using a closure.

What do others think?

/cc @pgn-vole @teohhanhui

@sebinsua
Copy link
Contributor Author

sebinsua commented Aug 28, 2018

Can anybody help me work out why the AppVeyor build isn't working?

Edit: It was a cross-environment problem in my test.

@sebinsua sebinsua force-pushed the feat/transform-sourcemap-paths branch 7 times, most recently from 6582e26 to dd94352 Compare August 29, 2018 22:47
@sebinsua sebinsua force-pushed the feat/transform-sourcemap-paths branch from dd94352 to 7901bb0 Compare August 29, 2018 23:01
@teohhanhui
Copy link

I do not think it's necessary to supply a base directory as was mentioned in the original GitHub issue. A transformation function can also handle making the path relative to a base directory. If you're able to pass a transform function in through a rollup.config.js you will also be able to pass a base path into your transformation function using a closure.

True, if they're all absolute paths, then it's fine.

@sebinsua
Copy link
Contributor Author

sebinsua commented Sep 3, 2018

True, if they're all absolute paths, then it's fine.

@teohhanhui Can you explain what you mean (or what you're trying to achieve), because in the unit test within this PR I have a relative path which I can then apply a transformation to.

e.g.

const path = require('path');
const pkg = require('./package.json');

module.exports = {
  description: 'transform sourcemap paths (#2168)',
  options: {
    output: {
      name: 'myModule',
      file: path.resolve(__dirname, '_actual', 'bundle.js'),
      sourcemapPathTransform: sourcePath => sourcePath.replace(
        // To demonstrate, sourcePath is a relative path and I am doing a string-replace on it. 
        `..${path.sep}`,
        // I'm replacing it with the package name followed by a slash.
        `${pkg.name}${path.sep}`
      )
    }
  }
};

@teohhanhui
Copy link

Sorry, I'm not familiar with how it works in rollup. But do see my gulp example in the previous issue to understand what I mean...

Anyway, as long as all the paths are already normalized to be relative to the same base directory, there's no problem.

@pgn-vole
Copy link

pgn-vole commented Sep 4, 2018

Looks good @sebinsua!
Thanks for that!

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much! Could not find any issues from my side, code looks good, too. Will put this into the next release!

addUnknownOptionErrors(
unknownOptionErrors,
Object.keys(command),
validInputOptions.concat(
validOutputOptions,
validCliOutputOptions,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thinking!

@lukastaegert
Copy link
Member

One note: Once this is released, it should be documented on the website. As this tends to be forgotten and we are somewhat short-staffed, if someone would open a PR on https://github.com/rollup/rollupjs.org adding this to the big list of options (maybe with a simple example where all files paths are changed to a different base path?) this would be a great help and I will merge it once we release.

Thanks!

@lukastaegert lukastaegert merged commit b67bf5b into rollup:master Sep 13, 2018
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

4 participants