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.

Sign up for GitHub

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

Jump to bottom

fix: replace type assertion with type guard #4302

Merged
merged 4 commits into from Dec 24, 2021
Merged

fix: replace type assertion with type guard #4302

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
18819c2
fix: replace type assertion with type guard
dnalborczyk Dec 14, 2021
3c82f6b
replace another assertion with type guard
dnalborczyk Dec 16, 2021
4b1bac7
Merge branch 'master' into bundle-type-fix
lukastaegert Dec 24, 2021
9d6866e
Review comments
lukastaegert Dec 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/Bundle.ts
View file
Open in desktop
Expand Up @@ -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(
Copy link
Member

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.

Copy link
Contributor Author

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. 🤷

(module): module is Module =>
module instanceof Module &&
(module.isIncluded() || module.info.isEntry || module.includedDynamicImporters.length > 0)
) as Module[];
);
}

function addModuleToManualChunk(
Expand Down
5 changes: 2 additions & 3 deletions src/utils/getOriginalLocation.ts
View file
Open in desktop
Expand Up @@ -4,10 +4,9 @@ export function getOriginalLocation(
sourcemapChain: DecodedSourceMapOrMissing[],
location: { column: number; line: number; name?: string; source?: string }
): { column: number; line: number } {
// This cast is guaranteed. If it were a missing Map, it wouldn't have a mappings.
const filteredSourcemapChain = sourcemapChain.filter(
sourcemap => sourcemap.mappings
) as ExistingDecodedSourceMap[];
(sourcemap): sourcemap is ExistingDecodedSourceMap => sourcemap.mappings !== undefined
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

);

while (filteredSourcemapChain.length > 0) {
const sourcemap = filteredSourcemapChain.pop()!;
Expand Down