fix(vendor): ignore import map in output directory instead of erroring #14998
Conversation
cli/args/mod.rs
Outdated
| @@ -122,6 +122,14 @@ impl RootConfig { | |||
| ) | |||
| } | |||
|
|
|||
| /// Removes the import map from the configuration. | |||
| pub fn remove_import_map(&mut self) { | |||
There was a problem hiding this comment.
Oh? This seems a bit strange to me, we are we modifying configuration from the process?
There was a problem hiding this comment.
I need to ensure that the import map is not used for anything. An easy way to do that is to modify this configuration to not specify it and then use this mutated configuration that doesn't specify an import map to build everything.
Actually, maybe RootOptions might have been a better name than RootConfig?
There was a problem hiding this comment.
I'll change this name to clear_import_map
There was a problem hiding this comment.
I need to ensure that the import map is not used for anything. An easy way to do that is to modify this configuration to not specify it and then use this mutated configuration that doesn't specify an import map to build everything.
Ok 👍
Actually, maybe
RootOptionsmight have been a better name thanRootConfig?
Yes, I think that might be a better name. Maybe CliOptions would be better?
There was a problem hiding this comment.
Yes, I think that might be a better name. Maybe CliOptions would be better?
Ok, I like CliOptions. I can do another PR after this one that renames it.
There was a problem hiding this comment.
I'll see when I go to work on it. I don't think it's necessary, but this was a decent stepping stone.
There was a problem hiding this comment.
So, one nice thing about doing it lazily is that we can surface errors only when necessary and can ignore errors in certain scenarios. For example, only displaying task errors when running a deno task or when using watch we could display an error about a certain part not working, but still continue execution.
There was a problem hiding this comment.
I see, that indeed seems interesting, but I'm worried about potential can of worms we could open. I'll leave it to you to make the decision.
There was a problem hiding this comment.
It already works lazily. Changing it to not be lazy could open a can of worms of more errors happening.
There was a problem hiding this comment.
@bartlomieju I added an "overrides" instead of clearing out the underlying structure. Let me know if this looks better than before.
| @@ -44,13 +44,19 @@ use crate::file_fetcher::CacheSetting; | |||
| use crate::lockfile::Lockfile; | |||
| use crate::version; | |||
|
|
|||
| #[derive(Default)] | |||
| struct CliOptionOverrides { | |||
There was a problem hiding this comment.
Seems good 👍 could use a doctring
This ignores an import map specified in the output directory instead of erroring.
Closes #14899