-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Sourcemap types #2985
Conversation
There was a problem hiding this 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/utils/collapseSourcemaps.ts
Outdated
this.sources = sources; | ||
this.names = map.names; | ||
this.mappings = map.mappings; | ||
this.mappings = map.mappings as SourceMapMappings; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 {
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/rollup/types.d.ts
Outdated
@@ -38,6 +38,16 @@ export interface RollupLogProps { | |||
url?: string; | |||
} | |||
|
|||
export interface ExistingDecodedSourceMap { | |||
file?: string; | |||
mappings: number[][][]; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
164b8d8
to
b682b3d
Compare
It has types now
f31c1d4
to
37ac3e9
Compare
`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)
`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)
This PR contains:
Are tests included?
Breaking Changes?
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).