-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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