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.

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

feat: add --no-check=remote flag #12766

Merged
merged 1 commit into from
Nov 29, 2021
Merged

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Nov 15, 2021

Closes #11970

This is an MVP feature that allows --no-check=remote to be passed on the command line, and will cause diagnostics related to remote modules to be "filtered" and not cause the process to terminate.

It uses a simplistic concept of remote modules at this point, which is those diagnostics where the specifier for the diagnostics starts with http. This means type errors in a remote module that import a data URL would not be covered. This would require an increased complexity of determining the importer of a specifier and determining if that importer is a remote module or not.

Also realised that I currently haven't included file names that start with asset or deno, which might also make sense, as users can't "control" errors from those locations.

If we ever considered this the default behaviour, we might want to consider a warning indicating that some diagnostics were filtered out and to use a different flag to have them displayed.

Comment on lines +2364 to +2371
if let Some(cache_type) = matches.value_of("no-check") {
match cache_type {
"remote" => flags.check = CheckFlag::Local,
_ => debug!(
"invalid value for 'no-check' of '{}' using default",
cache_type
),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use possible_values in flag definition instead of manually parsing? Or does it not play nicely with "no value"?

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'm in favor of this change, I agree it's better approach than #12726

@kitsonk
Copy link
Contributor Author

kitsonk commented Nov 28, 2021

Would be good to get @lucacasonato's view on this, since it was his idea.

@lucacasonato
Copy link
Member

Yeah, sorry. Will review Monday morning!

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried it out locally, and this works great. Thanks @kitsonk!

@kitsonk kitsonk merged commit 18a63dd into denoland:main Nov 29, 2021
@kitsonk kitsonk deleted the kitsonk/issue11970 branch November 29, 2021 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't report diagnostics for "external" TypeScript code
3 participants