fix: replace type assertion with type guard #4302
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4302 +/- ##
=======================================
Coverage 98.44% 98.44%
=======================================
Files 205 205
Lines 7316 7316
Branches 2084 2084
=======================================
Hits 7202 7202
Misses 55 55
Partials 59 59
Continue to review full report at Codecov.
|
src/Bundle.ts
Outdated
| @@ -306,11 +306,11 @@ function validateOptionsForMultiChunkOutput( | |||
| } | |||
|
|
|||
| function getIncludedModules(modulesById: Map<string, Module | ExternalModule>): Module[] { | |||
| return [...modulesById.values()].filter( | |||
| module => | |||
| return Array.from(modulesById.values()).filter( | |||
There was a problem hiding this comment.
Out of curiosity: Does Array.from(iterator) provide any typing benefits over [... iterator]? From their behaviour if you do not use the optional second argument of Array.from, I would expect them to be more or less the same with the spread operator possibly being slightly more efficient as it needs to do less checking.
There was a problem hiding this comment.
I think it's more or less the same. dunno about the runtime tho, as the former creates a new array instance and spreads the map values, while the latter only traverses the map. but all that might be optimized away anyways. 🤷
src/utils/getOriginalLocation.ts
Outdated
| const filteredSourcemapChain = sourcemapChain.filter( | ||
| sourcemap => sourcemap.mappings | ||
| ) as ExistingDecodedSourceMap[]; | ||
| (sourcemap): sourcemap is ExistingDecodedSourceMap => sourcemap.mappings !== undefined |
There was a problem hiding this comment.
Are you sure you want to explicitly compare against undefined and there will never be a null? The "good" type should be an object, so it would be ok to also filter out e.g. empty strings etc. I think I liked the previous check better.
There was a problem hiding this comment.
@lukastaegert uh, sorry, I think I came too late. :/
compare the original source code: sourcemap => sourcemap.mappings ---> no comparison at all, .mappings is just passed back as is
I addd it only because according to the types, mappings can be an array and undefined, and the undefined check makes the type checker happy (and is otherwise probably also correct. unless the typing is wrong.
export type DecodedSourceMapOrMissing =
| {
mappings?: never;
missing: true;
plugin: string;
}
| ExistingDecodedSourceMap;
I think I liked the previous check better.
there were none. ;)
if you think undefined is incorrect for mappings, that we should remove that type from the possibilities.
There was a problem hiding this comment.
The previous check was a check for truthiness, which you replaced with an explicit but much more strict one.
There was a problem hiding this comment.
the previous check seemed to be insufficient for typescript, otherwise I would have left it as is. 😉
https://github.com/rollup/rollup/runs/4639006105?check_suite_focus=true
I assume that filter expects a real boolean, not just a truthy/falsy value. (at least in the world of TS).
do you think the typing is incorrect then? can mappings be null as well? presumably a check for != null would be better? to check for falsy or thruthy values seems to be over checking for things. if mappings can only be an array or undefined (and possibly null) I don't think it's good pattern to check for anything else truthy (0, '', and what not) just in case, unless that case is valid. in that case tho, the typings might be incorrect.
I think it makes the code more readable and more robust. it's really hard to follow any code paths otherwise, specially if the code and the types don't match.
There was a problem hiding this comment.
I vaguely remember that mappings could be an empty string, but I am not sure in this instance. Moreover, anything could be in a user generated source map. I am just saying, the previous check was concise and worked well, also for TypeScript. TypeScript understands very well that filter does an implicit cast to Boolean and does not flag anything here. The new check might work as well, but it needlessly takes the risk of breaking stuff for other people, and I do not see a need to do so here.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description