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

fix sourcemap support #5

Merged
merged 6 commits into from
Aug 28, 2020
Merged

fix sourcemap support #5

merged 6 commits into from
Aug 28, 2020

Conversation

billowz
Copy link
Contributor

@billowz billowz commented Mar 4, 2019

  • Upgrade rollup to 1.0.0
  • Added mapContent option
  • Added sourcemap test case
  • Fix source path in the sourcemap

@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #5 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #5   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines          33     24    -9     
=====================================
- Hits           33     24    -9
Impacted Files Coverage Δ
src/proc-file.js 100% <100%> (ø) ⬆️
src/index.js 100% <0%> (ø) ⬆️
src/make-filter.js 100% <0%> (ø) ⬆️
src/parse-options.js 100% <0%> (ø) ⬆️

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 c6e7465...9eceb53. Read the comment docs.

@billowz billowz changed the title fix source path && support map content fix sourcemap support Mar 4, 2019
@aMarCruz
Copy link
Owner

aMarCruz commented Mar 5, 2019

@Tao-Zeng , I think it's better to put the mapContent option in a separate PR. Although I do not really think it's useful, as far as I know Rollup ignores the content put there by the plugins. Do you have any use case?

and good catch with "rm -rf" 👍 ...I forgot Windows exists

@billowz
Copy link
Contributor Author

billowz commented Mar 8, 2019

@aMarCruz Rollup1.x does not generate source content when the plugin's sourcemap does not contain source content

my test case

index.ts

//#if _DEBUG
export const a = 1
/*#else
export const a = 1
//#endif */

rollup config (rollup v1.3.2)

export default {
  input: 'src/index.ts',
  output: {
     name: 'test',
     format: 'umd',
     file: 'bundle.js',
     sourcemap: true,
     amd: {id: 'test'}
  },
  plugins: [
    nodeResolve({ jsnext: true, extensions: ['.ts'] }),
    commonjs(),
    babel({extensions: ['.ts'] }),
    jscc({ values: { _DEBUG: true } })
  ]
}

bundle.js.map

{"version":3,"file":"bundle.js","sources":["./src/src/index.ts"],"sourcesContent":[null],"names":["a"],"mappings":";;;;;;;;;;;;;;;AAAA;;;;;;;;;;;;;;AAeA,MAAaA,CAAC,GAAG,CAAV;;;;"}
  1. sourcesContent is [null]
  2. sources expect ["./src/index.ts"]

@billowz
Copy link
Contributor Author

billowz commented Mar 8, 2019

@aMarCruz The mapContent option is a better idea to work with the sourceupExcludeSources option of the rollup

@aMarCruz
Copy link
Owner

aMarCruz commented Mar 8, 2019

@Tao-Zeng I will check it asap. Thanks.

@billowz billowz force-pushed the master branch 3 times, most recently from 9f3d183 to 9cba7b9 Compare June 19, 2019 11:55
@billowz
Copy link
Contributor Author

billowz commented Jun 20, 2019

@aMarCruz I have rewritten the sourcemap fix code

  1. Upgrade rollup to 1.0.0
  2. Added mapContent option
  3. Added sourcemap test case
  4. Fix source path in the sourcemap

The source path is incorrect for the following reasons:

The source path in the sourcemap returned by jscc is the path relative to process.pwd(), rollup will resolve source path through the file's directory absolute path and the source path in the returned source map by the plugin

const sources = originalSourcemap.sources;
const sourcesContent = originalSourcemap.sourcesContent || [];

if (sources == null || (sources.length <= 1 && sources[0] == null)) {
	source = new Source(module.id, sourcesContent[0]);
	sourcemapChain = [originalSourcemap as RawSourceMap].concat(sourcemapChain);
} else {
	// TODO indiscriminately treating IDs and sources as normal paths is probably bad.
	const directory = dirname(module.id) || '.';
	const sourceRoot = originalSourcemap.sourceRoot || '.';
        
        // parse the source path in sourcemap
	const baseSources = sources.map(
		(source, i) => new Source(resolve(directory, sourceRoot, source), sourcesContent[i])
	);
	
	source = new Link(originalSourcemap as any, baseSources) as any;
}

See the rollup source: https://github.com/rollup/rollup/blob/v1.15.6/src/utils/collapseSourcemaps.ts#L196

I hope that you can check and merge the RP, Thanks.

@atompkins
Copy link

What is the status on this please?

@aMarCruz
Copy link
Owner

@atompkins , merged, I will publish a new version this week.
@billowz thanks and I apologize for the long delay.

@aMarCruz aMarCruz merged commit 24bc2bb into aMarCruz:master Aug 28, 2020
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

3 participants