Introduce an aggregate resolver

[kitsonk]

Just a thought and maybe not for this review, but I wonder if we should have some other AggregateResolver or something that takes a collection of dyn Resolvers and then tries to resolve with each resolver in order. That way in the consuming code someone could build one up and it might be a bit more clear about what's going on.

Originally posted by @dsherret in #12631 (comment)

[kitsonk]

@bartlomieju I think this would help with the "compat" resolution/resolver as well to allow it to support import maps.

[bartlomieju]

This is an interesting idea, but I'm not sure if it's feasible to go through the resolvers one by one. As an example, let's discuss current implementation of NodeEsmResolver. This resolver internally owns ImportMapResolver and here's a relevant code that actually implements resolution:

impl Resolver for NodeEsmResolver {
  fn resolve(
    &self,
    specifier: &str,
    referrer: &ModuleSpecifier,
  ) -> Result<ModuleSpecifier, AnyError> {
    // First try to resolve using import map, ignoring any errors
    if !specifier.starts_with("node:") {
      if let Some(import_map_resolver) = &self.maybe_import_map_resolver {
        if let Ok(specifier) = import_map_resolver.resolve(specifier, referrer)
        {
          return Ok(specifier);
        }
      }
    }

    let node_resolution =
      node_resolve(specifier, referrer.as_str(), &std::env::current_dir()?);

    match node_resolution {
      Ok(specifier) => {
        // If node resolution succeeded, return the specifier
        Ok(specifier)
      }
      Err(err) => {
        // If node resolution failed, check if it's because of unsupported
        // URL scheme, and if so try to resolve using regular resolution algorithm
        if err
          .to_string()
          .starts_with("[ERR_UNSUPPORTED_ESM_URL_SCHEME]")
        {
          return deno_core::resolve_import(specifier, referrer.as_str())
            .map_err(|err| err.into());
        }

        Err(err)
      }
    }
  }
}

As you can see above it first tries to resolve using an ImportMapResolver (if it is defined), but only for specifiers that do not start with node: prefix (because those should be handled by NodeEsmResolver). Additionally if "node" resolution fails we fallback to regular "deno" resolution, but only if "node" resolution failed due to unsupported scheme (ie. errored with [ERR_UNSUPPORTED_ESM_URL_SCHEME], which happens for http:, http: and blob: schemes), all other errors should IMO be surfaced to the user.

I'm curious to hear about proposed solution, but I don't believe we could just go through a list of resolvers one-by-one if we need to branch out on different prefixes and errors.

[kitsonk]

It would be a breaking change, but if resolve() returned Option<ModuleSpecifier> instead, then you would go down the stack until you got a response, with the aggregate specifier always returning Some<ModuleSpecifier>

[bartlomieju]

It would be a breaking change, but if resolve() returned Option<ModuleSpecifier> instead, then you would go down the stack until you got a response, with the aggregate specifier always returning Some<ModuleSpecifier>

@kitsonk how would situation described in the comment above be handled then? It's not as simple as going through the stack as there are some conditionals needed to branch into different resolvers.

[kitsonk]

If a resolver is supposed to handle it, it returns a Some, otherwise a None. In what scenario does that not work? You can't have a fight between resolver, you have to have a precedence.

[castarco]

Are these aggregate resolvers a proposal to implement something like this?
#12213 (comment)

btw, I believe that there is still plenty of space to improve the "static" nature of the current modules resolver, without having to jump already into "dynamic machinery" (conditionals, runtime-computed module names, etc.), which may introduce too many corner cases.

[bartlomieju]

@castarco no, this issue is about refactoring already existing resolvers in the codebase and has no impact on the proposal you linked.

[castarco]

@bartlomieju I think this would help with the "compat" resolution/resolver as well to allow it to support import maps.

oh, ok. thx!

[bartlomieju]

@bartlomieju I think this would help with the "compat" resolution/resolver as well to allow it to support import maps.

oh, ok. thx!

We already support import maps, it's just they are somewhat bolted on in the resolution flow, which this issue suggests to refactor. At the moment there are no plans to add non-standard features to import maps. We can endorse proposals that make sense in Deno, but we don't want to diverge from the spec (eg. That's why we don't allow for jsonc files for import maps)

[bartlomieju]

I'm gonna close this issue, as with recent refactors we got rid of multiple resolvers.