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

Sourcemap types #2985

Merged
merged 17 commits into from Jul 7, 2019
Merged

Sourcemap types #2985

merged 17 commits into from Jul 7, 2019

Conversation

jridgewell
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes
  • no

Breaking Changes?

  • yes
  • no

List any relevant issue numbers:

Description

This cleans up the types surrounding source maps, getting rid of most of the typecasts (and all of the any casts).

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.

Thanks so much, the bad source map types have really been bugging me, this is a great improvement! Just a small suggestion to get rid of some of the remaining casts, otherwise this is good to be merged.

src/rollup/types.d.ts Outdated Show resolved Hide resolved
this.sources = sources;
this.names = map.names;
this.mappings = map.mappings;
this.mappings = map.mappings as SourceMapMappings;
Copy link
Member

Choose a reason for hiding this comment

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

I believe this type casts and many of the other remaining type casts can be avoided if instead of using SourceMapMappings, SourceMapLine and SourceMapSegment we internally use number[][][], number[][] and `number[]. To make this clearer, we could introduce our own types to encapsulate these. This would get rid of the type casts here and in lines 61, 96, and 128, and would require using our own types in lines 33, 51, and 82.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SourceMapSegment is important, though, because it's a tuple of 1, 4, or 5 numbers. The added line on 54 (if (segment.length == 1) continue) was enforced because typescript knows there might not be a 2nd number to access.

I tried switching all sites to use SourceMapMappings, but magic-string's DecodeSourceMap type uses number[][][]. We could try fixing magic-string's type? Or casting it into the correct tuple type ExistingDecodedSourceMap. Something like:

Proper tuple types
diff --git a/src/rollup/types.d.ts b/src/rollup/types.d.ts
index c63653e4..4ac4e2c9 100644
--- a/src/rollup/types.d.ts
+++ b/src/rollup/types.d.ts
@@ -1,5 +1,6 @@
 import * as ESTree from 'estree';
 import { EventEmitter } from 'events';
+import { SourceMapMappings } from 'sourcemap-codec';
 
 export const VERSION: string;
 
@@ -40,7 +41,7 @@ export interface RollupLogProps {
 
 export interface ExistingDecodedSourceMap {
 	file?: string;
-	mappings: number[][][];
+	mappings: SourceMapMappings;
 	names: string[];
 	sourceRoot?: string;
 	sources: string[];
diff --git a/src/utils/collapseSourcemaps.ts b/src/utils/collapseSourcemaps.ts
index 5face4b5..c9b377b2 100644
--- a/src/utils/collapseSourcemaps.ts
+++ b/src/utils/collapseSourcemaps.ts
@@ -2,7 +2,7 @@ import { DecodedSourceMap, SourceMap } from 'magic-string';
 import { SourceMapLine, SourceMapMappings, SourceMapSegment } from 'sourcemap-codec';
 import Chunk from '../Chunk';
 import Module from '../Module';
-import { DecodedSourceMapOrMissing } from '../rollup/types';
+import { DecodedSourceMapOrMissing, ExistingDecodedSourceMap } from '../rollup/types';
 import { error } from './error';
 import { basename, dirname, relative, resolve } from './path';
 
@@ -34,10 +34,10 @@ class Link {
 	names: string[];
 	sources: (Source | Link)[];
 
-	constructor(map: { mappings: number[][][]; names: string[] }, sources: (Source | Link)[]) {
+	constructor(map: { mappings: SourceMapMappings; names: string[] }, sources: (Source | Link)[]) {
 		this.sources = sources;
 		this.names = map.names;
-		this.mappings = map.mappings as SourceMapMappings;
+		this.mappings = map.mappings;
 	}
 
 	traceMappings() {
@@ -58,7 +58,7 @@ class Link {
 				const traced = source.traceSegment(
 					segment[2],
 					segment[3],
-					this.names[segment[4] as number]
+					segment.length === 5 ? this.names[segment[4]] : ''
 				);
 
 				if (traced) {
@@ -125,7 +125,7 @@ class Link {
 				return source.traceSegment(
 					segment[2],
 					segment[3],
-					this.names[segment[4] as number] || name
+					segment.length === 5 ? this.names[segment[4]] : name
 				);
 			}
 			if (segment[0] > column) {
@@ -197,7 +197,9 @@ export default function collapseSourcemaps(
 			return source;
 		});
 
-	let source = new Link(map, moduleSources);
+	// DecodedSourceMap (from magic-string) uses a number[] instead of the more
+	// correct SourceMapSegment tuples. Cast it here to gain type safety.
+	let source = new Link(map as ExistingDecodedSourceMap, moduleSources);
 
 	source = bundleSourcemapChain.reduce(linkMap, source);
 
diff --git a/src/utils/decodedSourcemap.ts b/src/utils/decodedSourcemap.ts
index ecc54f17..3b4acfcf 100644
--- a/src/utils/decodedSourcemap.ts
+++ b/src/utils/decodedSourcemap.ts
@@ -18,7 +18,7 @@ export function decodedSourcemap(map: Input): ExistingDecodedSourceMap | null {
 		};
 	}
 
-	let mappings: number[][][];
+	let mappings;
 	if (typeof map.mappings === 'string') {
 		mappings = decode(map.mappings);
 	} else {

Copy link
Member

Choose a reason for hiding this comment

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

That would be an even better solution. Only point is that I would like to avoid an external dependency from types.d.ts to sourcemap-codec as this would introduce a completely new external dependency onto the published bundle just for its types. Instead, I would prefer to just copy the types, as few as they are. Improving the types of magic-string would definitely be a good idea, also considering both sourcemap-codec and magic-string are originally from the same author. Until this is done, your suggestion is a good approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -38,6 +38,16 @@ export interface RollupLogProps {
url?: string;
}

export interface ExistingDecodedSourceMap {
file?: string;
mappings: number[][][];
Copy link
Member

Choose a reason for hiding this comment

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

cf. my comment in collapseSourceMaps, if we introduce our own named types, this should use the mappings type as well.

export type RawSourceMap = { mappings: '' } | ExistingRawSourceMap;
export type DecodedSourceMapOrMissing =
| {
mappings?: never;
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -12,7 +12,6 @@ declare module 'signal-exit';
declare module 'date-time';
declare module 'locate-character';
declare module 'is-reference';
declare module 'sourcemap-codec';
Copy link
Member

Choose a reason for hiding this comment

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

👍

jridgewell added a commit to jridgewell/magic-string that referenced this pull request Jul 6, 2019
`SourceMapSegment`s aren't really `number[]` types, they're tuples of 1, 4, or 5 numbers.

See the discussion we're having at rollup/rollup#2985 (comment)
@lukastaegert lukastaegert merged commit e66d7be into rollup:master Jul 7, 2019
@jridgewell jridgewell deleted the sourcemap-types branch July 7, 2019 10:43
mourner pushed a commit to Rich-Harris/magic-string that referenced this pull request Oct 4, 2019
`SourceMapSegment`s aren't really `number[]` types, they're tuples of 1, 4, or 5 numbers.

See the discussion we're having at rollup/rollup#2985 (comment)
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