Improve DependencyDescriptor to combine import_assertions with dynamic_import_assertions

[dsherret]

Describe the feature

The type of DependencyDescriptor is error prone because it does not force you to check the correct value:

struct DependencyDescriptor {
    // ...etc...

    /// Import assertions for this dependency.
    /// NOTE: it's filled only for static imports and exports.
    pub import_assertions: HashMap<String, String>,
    /// Import assertions for dynamic imports. Present if `is_dynamic` is `true`
    /// and `kind` is not `DependencyKind::Require`.
    pub dynamic_import_assertions: Option<DynamicImportAssertions>,
}

#[derive(Clone, Debug, Eq, PartialEq)]
pub enum DynamicImportAssertions {
    /// The set of assertion keys could not be statically analyzed.
    Unknown,
    /// The set of assertion keys is statically analyzed, though each respective
    /// value may or may not not be.
    Known(HashMap<String, DynamicImportAssertion>),
}

Instead it would be better to do something along the lines of:

struct DependencyDescriptor {
    // ...etc...

    /// Import assertions for this dependency when not `DependencyKind::Require`.
    pub import_assertions: Option<ImportAssertions>,
}

#[derive(Clone, Debug, Eq, PartialEq)]
pub enum ImportAssertions {
    /// The set of assertion keys could not be statically analyzed.
    Unknown,
    /// The set of assertion keys is statically analyzed, though each respective
    /// value may or may not not be for dynamic imports.
    Known(HashMap<String, ImportAssertion>),
}

#[derive(Clone, Debug, Eq, PartialEq)]
pub enum ImportAssertion {
    /// The value of this assertion could not be statically analyzed.
    Unknown,
    /// The value of this assertion is a statically analyzed string.
    Known(String),
}

Babel plugin or link to the feature description

No response

Additional context

No response

[nayeemrmn]

In addition I had second thoughts about letting individual keys be unknown, instead of just requiring that the whole object is a simple literal or set as unknown. That would make it much less annoying to unwrap Known() assertions for static imports (which is what I crudely tried to avoid with the current API).

We could just use an Option:

struct DependencyDescriptor {
    /// Import assertions. Set as `None` for dynamic imports whose assertions are not statically analyzable.
    pub import_assertions: Option<HashMap<String, String>>,
}

[dsherret]

@nayeemrmn I think it might be useful to still have all the detail, but to make it easy to use the API for common cases perhaps we could add helper methods instead? So maybe something like:

impl ImportAssertions {
  pub fn get(&self, key: &str) -> Option<&String> {
    // ...
  }
}

...then we can just make it pub import_assertions: ImportAssertions (it would just be empty for "require") so you would do dep.import_assertions.get("type") similar to before.

[dsherret]

By the way, @nayeemrmn would you be willing to make this change? If not, I will get to it within a few days.

[dsherret]

I'm starting work on this.

[swc-bot]

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.