Compose sourcemaps #291
Open
Compose sourcemaps #291
+117
−21
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideAdds composition of bundler and javascript-obfuscator source maps using @jridgewell/remapping, wires it through both main and worker obfuscation paths, and introduces targeted tests and dependencies to validate correct source content tracing. Sequence diagram for obfuscation and composed source map generationsequenceDiagram
actor Bundler
participant Plugin as ObfuscationPlugin
participant Worker as ObfuscationWorker
participant JSObfuscator as JavascriptObfuscator
participant Remap as Remapping
Bundler->>Plugin: obfuscateBundle(bundleItem)
alt main_thread_obfuscation
Plugin->>JSObfuscator: obfuscate(bundleItem.code, options)
JSObfuscator-->>Plugin: ObfuscationResult
Plugin->>Remap: composeSourcemaps(bundleItem.map, ObfuscationResult.map)
Remap-->>Plugin: composedMap
Plugin-->>Bundler: { code, map: composedMap }
else worker_based_obfuscation
Plugin->>Worker: postMessage(bundleItem, options)
Worker->>JSObfuscator: obfuscate(bundleItem.code, options)
JSObfuscator-->>Worker: ObfuscationResult
Worker->>Remap: composeSourcemaps(bundleItem.map, ObfuscationResult.map)
Remap-->>Worker: composedMap
Worker-->>Plugin: { fileName, obfuscatedCode, map: composedMap }
Plugin-->>Bundler: aggregated obfuscation results
end
Class diagram for updated utilities with composeSourcemapsclassDiagram
class Utils {
+getChunkName(id string, manualChunks string[]) string
+composeSourcemaps(map1 Rollup.SourceMapInput, map2 Rollup.SourceMapInput, log function) Rollup.SourceMapInput
+obfuscateBundle(finalConfig Config, fileName string, bundleItem any) ObfuscationResult
}
class ObfuscatedFilesRegistry {
-instance ObfuscatedFilesRegistry
-obfuscatedFiles Set~string~
+getInstance() ObfuscatedFilesRegistry
+markAsObfuscated(fileName string) void
+isObfuscated(fileName string) boolean
}
class WorkerModule {
+handleMessage(message WorkerMessage) void
}
class JavascriptObfuscator {
+obfuscate(code string, options ObfuscatorOptions) ObfuscationResult
}
class Remapping {
+remapping(maps SourceMapInput[], loader function) SourceMapInput
}
Utils --> ObfuscatedFilesRegistry : uses
Utils --> JavascriptObfuscator : uses
Utils --> Remapping : uses composeSourcemaps
WorkerModule --> Utils : uses composeSourcemaps
WorkerModule --> JavascriptObfuscator : uses
ObfuscatedFilesRegistry <.. Utils : singleton access
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
JSON.parse(JSON.stringify(bundleItem.map) || 'null')pattern is duplicated in bothobfuscateBundleand the worker; consider extracting this into a small helper (e.g.toPlainSourceMapInput) so the intent of stripping methods is clearer and easier to maintain. composeSourcemapscurrently relies on casting betweenRollup.SourceMapInputandremapping.SourceMapInput; it would be safer to tighten the types (e.g. via a shared alias or adapter) so you don't needascasts andanyin tests, and to make it explicit which properties are actually required byremapping.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `JSON.parse(JSON.stringify(bundleItem.map) || 'null')` pattern is duplicated in both `obfuscateBundle` and the worker; consider extracting this into a small helper (e.g. `toPlainSourceMapInput`) so the intent of stripping methods is clearer and easier to maintain.
- `composeSourcemaps` currently relies on casting between `Rollup.SourceMapInput` and `remapping.SourceMapInput`; it would be safer to tighten the types (e.g. via a shared alias or adapter) so you don't need `as` casts and `any` in tests, and to make it explicit which properties are actually required by `remapping`.
## Individual Comments
### Comment 1
<location> `src/__tests__/plugin.spec.ts:243-240` </location>
<code_context>
+describe('composeSourcemaps', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for null and single-map inputs to cover all branches of `composeSourcemaps`.
The current test only covers the case where both maps are present. To also cover the early-return branches, please add tests for:
- `composeSourcemaps(null, null, logSpy)` → returns `null` and does not log.
- `composeSourcemaps(bundlerMap, null, logSpy)` → returns `bundlerMap` and does not log.
- `composeSourcemaps(null, obfuscatorMap, logSpy)` → returns `obfuscatorMap` and does not log.
This will exercise all branches and verify behavior when one or both inputs are missing.
Suggested implementation:
```typescript
describe('composeSourcemaps', () => {
it('should compose maps and preserve the original source content', () => {
const originalSource = 'const message = "hi";\nconsole.log(message);\n';
const intermediateSource = 'const message="hi";\nconsole.log(message);\n';
const bundlerMap: Rollup.SourceMapInput = {
version: 3,
file: 'bundle.js',
sources: ['original.ts'],
sourcesContent: [originalSource],
names: [],
// ...rest of existing test setup and assertions...
};
// ...existing expectations for composed sourcemap...
});
it('returns null and does not log when both maps are null', () => {
const logSpy = vi.fn();
// @ts-expect-error testing null inputs
const result = composeSourcemaps(null, null, logSpy);
expect(result).toBeNull();
expect(logSpy).not.toHaveBeenCalled();
});
it('returns bundlerMap and does not log when obfuscator map is null', () => {
const logSpy = vi.fn();
const originalSource = 'const message = "hi";\nconsole.log(message);\n';
const bundlerMap: Rollup.SourceMapInput = {
version: 3,
file: 'bundle.js',
sources: ['original.ts'],
sourcesContent: [originalSource],
names: [],
mappings: '',
};
// @ts-expect-error testing null inputs
const result = composeSourcemaps(bundlerMap, null, logSpy);
expect(result).toBe(bundlerMap);
expect(logSpy).not.toHaveBeenCalled();
});
it('returns obfuscatorMap and does not log when bundler map is null', () => {
const logSpy = vi.fn();
const originalSource = 'const message = "hi";\nconsole.log(message);\n';
const obfuscatorMap: Rollup.SourceMapInput = {
version: 3,
file: 'bundle.obfuscated.js',
sources: ['bundle.js'],
sourcesContent: [originalSource],
names: [],
mappings: '',
};
// @ts-expect-error testing null inputs
const result = composeSourcemaps(null, obfuscatorMap, logSpy);
expect(result).toBe(obfuscatorMap);
expect(logSpy).not.toHaveBeenCalled();
});
```
I only see the beginning of the `composeSourcemaps` test; you'll need to:
1. Replace the `// ...rest of existing test setup and assertions...` and `// ...existing expectations for composed sourcemap...` comments with the original body of the existing test.
2. Ensure `composeSourcemaps` and `Rollup` are correctly imported at the top of the file (they likely already are).
3. If your `composeSourcemaps` function has a different parameter order or logging interface, adjust the calls and `logSpy` expectations accordingly.
</issue_to_address>
### Comment 2
<location> `src/utils/index.ts:193` </location>
<code_context>
- code: obfuscationResult.getObfuscatedCode(),
- map: sourceMap ? JSON.parse(sourceMap) : null,
+ code: obfuscated.getObfuscatedCode(),
+ map: composeSourcemaps(
+ JSON.parse(JSON.stringify(bundleItem.map) || 'null'), // strip methods
+ JSON.parse(obfuscated.getSourceMap() || 'null'),
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting source map normalization into a helper and simplifying the composeSourcemaps API to make sourcemap handling clearer and less incidental-complex.
You can reduce the incidental complexity around sourcemap handling with a couple of small refactors, without changing behavior.
### 1. Extract the JSON clone/normalization into a helper
Right now `obfuscateBundle` has an opaque one-liner and asymmetry:
```ts
map: composeSourcemaps(
JSON.parse(JSON.stringify(bundleItem.map) || 'null'), // strip methods
JSON.parse(obfuscated.getSourceMap() || 'null'),
_log.info.bind(_log),
),
```
Pull the cloning/normalization into a dedicated helper and use it symmetrically:
```ts
function toPlainSourceMap(map: Rollup.SourceMapInput | null): Rollup.SourceMapInput | null {
if (!map) return null;
// strip methods / non-enumerables via JSON round‑trip
return JSON.parse(JSON.stringify(map));
}
```
Then in `obfuscateBundle`:
```ts
_log.info('composing source maps...');
return {
code: obfuscated.getObfuscatedCode(),
map: composeSourcemaps(
toPlainSourceMap(bundleItem.map),
toPlainSourceMap(obfuscated.getSourceMap() as any), // still normalized
),
};
```
This keeps the intent clear, documents the tradeoff in one place, and removes the `|| 'null'` trick.
### 2. Simplify `composeSourcemaps`’s API (drop logger injection)
The `log?: (msg: string) => void` and `bind` at the call site add indirection for little gain. If you’re already logging in `obfuscateBundle` (as above), you can drop logging from `composeSourcemaps` entirely:
```ts
export function composeSourcemaps(
map1: Rollup.SourceMapInput | null,
map2: Rollup.SourceMapInput | null,
): Rollup.SourceMapInput | null {
if (!map1) return map2;
if (!map2) return map1;
const composed = remapping(
[map2 as remapping.SourceMapInput, map1 as remapping.SourceMapInput],
() => null,
);
return composed as Rollup.SourceMapInput;
}
```
Call site stays simple:
```ts
_log.info('composing source maps...');
map: composeSourcemaps(
toPlainSourceMap(bundleItem.map),
toPlainSourceMap(obfuscated.getSourceMap() as any),
),
```
### 3. Make the type casting intent explicit
If you keep the casts, a minimal comment helps future readers understand the compatibility assumption:
```ts
const composed = remapping(
// Rollup sourcemaps are structurally compatible with remapping.SourceMapInput
[map2 as remapping.SourceMapInput, map1 as remapping.SourceMapInput],
() => null,
);
return composed as Rollup.SourceMapInput;
```
Alternatively, you can type `composeSourcemaps` in terms of `remapping.SourceMapInput` and convert only at the edges, but the small comment above is likely enough and keeps the surface API in terms of Rollup.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| it('should return vendor modules when no match found', () => { | ||
| const manualChunks = ['react', 'vue']; | ||
| expect(getChunkName('node_modules/lodash/index.js', manualChunks)).toBe('vendor-modules'); | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Add tests for null and single-map inputs to cover all branches of composeSourcemaps.
The current test only covers the case where both maps are present. To also cover the early-return branches, please add tests for:
composeSourcemaps(null, null, logSpy)→ returnsnulland does not log.composeSourcemaps(bundlerMap, null, logSpy)→ returnsbundlerMapand does not log.composeSourcemaps(null, obfuscatorMap, logSpy)→ returnsobfuscatorMapand does not log.
This will exercise all branches and verify behavior when one or both inputs are missing.
Suggested implementation:
describe('composeSourcemaps', () => {
it('should compose maps and preserve the original source content', () => {
const originalSource = 'const message = "hi";\nconsole.log(message);\n';
const intermediateSource = 'const message="hi";\nconsole.log(message);\n';
const bundlerMap: Rollup.SourceMapInput = {
version: 3,
file: 'bundle.js',
sources: ['original.ts'],
sourcesContent: [originalSource],
names: [],
// ...rest of existing test setup and assertions...
};
// ...existing expectations for composed sourcemap...
});
it('returns null and does not log when both maps are null', () => {
const logSpy = vi.fn();
// @ts-expect-error testing null inputs
const result = composeSourcemaps(null, null, logSpy);
expect(result).toBeNull();
expect(logSpy).not.toHaveBeenCalled();
});
it('returns bundlerMap and does not log when obfuscator map is null', () => {
const logSpy = vi.fn();
const originalSource = 'const message = "hi";\nconsole.log(message);\n';
const bundlerMap: Rollup.SourceMapInput = {
version: 3,
file: 'bundle.js',
sources: ['original.ts'],
sourcesContent: [originalSource],
names: [],
mappings: '',
};
// @ts-expect-error testing null inputs
const result = composeSourcemaps(bundlerMap, null, logSpy);
expect(result).toBe(bundlerMap);
expect(logSpy).not.toHaveBeenCalled();
});
it('returns obfuscatorMap and does not log when bundler map is null', () => {
const logSpy = vi.fn();
const originalSource = 'const message = "hi";\nconsole.log(message);\n';
const obfuscatorMap: Rollup.SourceMapInput = {
version: 3,
file: 'bundle.obfuscated.js',
sources: ['bundle.js'],
sourcesContent: [originalSource],
names: [],
mappings: '',
};
// @ts-expect-error testing null inputs
const result = composeSourcemaps(null, obfuscatorMap, logSpy);
expect(result).toBe(obfuscatorMap);
expect(logSpy).not.toHaveBeenCalled();
});I only see the beginning of the composeSourcemaps test; you'll need to:
- Replace the
// ...rest of existing test setup and assertions...and// ...existing expectations for composed sourcemap...comments with the original body of the existing test. - Ensure
composeSourcemapsandRollupare correctly imported at the top of the file (they likely already are). - If your
composeSourcemapsfunction has a different parameter order or logging interface, adjust the calls andlogSpyexpectations accordingly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
javascript-obfuscatordoes not support taking an input source map to base its output source map on. Considering we have access tobundleItem.map, this PR composes/merges the input source map from bundler with the source map generated byjavascript-obfuscatorvia@jridgewell/remapping.Tested this PR on a production codebase for Sentry source maps, works as expected.
I haven't created a config entry for this because I don't see a use case where you wouldn't want this. If you're using Vite you already get nice source map composition out of the box, and this plugin shouldn't break that expectation.
Summary by Sourcery
Compose obfuscator-generated source maps with the original bundle source maps to preserve correct source mapping through the obfuscation step.
Enhancements:
Build:
Tests: