chore(types): updating typings for merge-source-map#3040
Conversation
document the typings for merge-source-map
there exist many sourcemap types within this PR. This attempts to eliminate one of them
| // need to merge sourcemaps at this point | ||
| const mergeMap = sourceMapMerge( | ||
| (minifyOpts.sourceMap as SourceMapOptions).content as SourceMap, | ||
| (minifyOpts.sourceMap as SourceMapOptions)?.content as SourceMap, |
There was a problem hiding this comment.
minifyOpts.sourceMap is typed as sourceMap?: boolean | SourceMapOptions;. This doesn't fix the type assertion usage (that's for a later day), but it does prevent accessing a field on an undefined value
| * @returns the merged sourcemap, or undefined if both maps are falsy | ||
| */ | ||
| function merge( | ||
| oldMap: import('../src/declarations').SourceMap, |
There was a problem hiding this comment.
If there's a better way to do this, I'm very much in favor of it (even if reintroducing the type declaration duplication is the route we take)
There was a problem hiding this comment.
I'm curious how this affects the browser... seems unrelated but I've been bitten by this import syntax. Take a peek at the /test/browser-compile directory and if that doesn't have any errors, I'm good!
There was a problem hiding this comment.
Looks good! Gonna put up a separate PR to add a reminder to TAL at that when doing a release until we get testing underway there
| * recreated here for type safety purposes. | ||
| */ | ||
| declare module 'merge-source-map' { | ||
| interface SourceMap { |
There was a problem hiding this comment.
There are so many representations of SourceMaps in the codebase. One from:
- terser
- MagicString
- Stencil
- rollup
This is an attempt to coalesce them, because honestly it gets confusing really quickly which definition you're dealing with. I don't want to combine them all into one definition, but this particular definition seemed extraneous
splitinfinities
left a comment
There was a problem hiding this comment.
LGTM if the /test/browser-compile directory doesn't have any errors.
this PR makes changes to the Stencil-defined type declaration file for `merge-source-map` `merge-source-map` does not offer any `d.ts` file, nor does it have a third party offering on Definitely Typed, so the intent of this effort was to see "how correct is our type definition for our use case, while not deviating from the actual library's implicit typings".
this PR makes changes to the Stencil-defined type declaration file for
merge-source-mapmerge-source-mapdoes not offer anyd.tsfile, nor does it have a third party offering on Definitely Typed, so the intent of this effort was to see "how correct is our type definition for our use case, while not deviating from the actual library's implicit typings".I did try to ask the compiler to generate the
d.tsfile for me withtsc -d node_modules/merge-source-map/index.js --allowJs --outDir ./out, which generated invalid TS:(the invalid part being you can't have a non-optional type following an optional one). I can always tweak that and use that if folks prefer it.