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 sourcemapBaseUrl option #4527

Merged

Conversation

nickgarlis
Copy link
Contributor

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 relevant issue numbers:

Description

This feature can be useful for when sourcemaps need to be loaded through a different URL to the one where the bundled code is loaded from.

It would be the equivalent of the publicPath option in Webpack's source-map-dev-tool plugin

The use case would be to source my organisation's sourcemaps in a server within our internal network to prevent exposing the source code on production.

If this is something of interest then the following features could also be considered:

  • Providing a base URL option for bundle sources (sources field inside of *.map files)
  • Providing a different output directory for sourcemaps.

@nickgarlis nickgarlis marked this pull request as ready for review June 10, 2022 13:39
@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Merging #4527 (b922bc3) into master (fbb86f5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4527   +/-   ##
=======================================
  Coverage   98.86%   98.87%           
=======================================
  Files         208      209    +1     
  Lines        7334     7346   +12     
  Branches     2095     2098    +3     
=======================================
+ Hits         7251     7263   +12     
  Misses         27       27           
  Partials       56       56           
Impacted Files Coverage Δ
src/utils/options/mergeOptions.ts 100.00% <ø> (ø)
src/rollup/rollup.ts 100.00% <100.00%> (ø)
src/utils/options/normalizeOutputOptions.ts 100.00% <100.00%> (ø)
src/utils/url.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbb86f5...b922bc3. Read the comment docs.

@nickgarlis nickgarlis force-pushed the add-sourcemap-base-url-option branch from 1d7097f to 81dde77 Compare July 1, 2022 12:47
@@ -0,0 +1,8 @@
export function isValidUrl(url: string): boolean {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure where I should add tests for functions like these. It looks like all of the tests in the project are end-to-end tests and do not import typescript code from src. @lukastaegert I'd be happy to open a separate PR adding unit tests that execute with mocha -r ts-node/register.

Copy link
Member

Choose a reason for hiding this comment

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

No, please don't. Then we would still need a test that actually uses the option, i.e. an E2E test. I mean that is the point, to test the effect of this option. As e2e tests in Rollup are REALLY fast, there is really no point in writing unit tests that need to be adapted with every refactoring.
A very simple way could be to add a chunking-form test because those tests just snapshot the output directory and that would include the source map. Or you add a sourcemap test, those tests pass you the source map and you make assertions on the generated map. If all else fails, I will need to look into writing a test myself.

@lukastaegert
Copy link
Member

I took the liberty to add a test for the bad case, which also prompted me to improve the error message a little. I also added some documentation for the option itself and shortened the CLI printout to match the 80 character line-break we use in that file. Otherwise, this is really high quality work and I plan to merge it once everything is green.

@lukastaegert lukastaegert enabled auto-merge (squash) July 8, 2022 07:54
@lukastaegert lukastaegert merged commit 768534a into rollup:master Jul 8, 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