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

Add ModuleKind to graph and be returned by Resolver.resolve #97

Closed
dsherret opened this issue Jan 6, 2022 · 5 comments · Fixed by #107
Closed

Add ModuleKind to graph and be returned by Resolver.resolve #97

dsherret opened this issue Jan 6, 2022 · 5 comments · Fixed by #107
Assignees
Labels
enhancement New feature or request

Comments

@dsherret
Copy link
Member

dsherret commented Jan 6, 2022

Was talking to @bartlomieju today so this is a quick summary of the discussion. For denoland/deno#12648 we'll need a way to identify if .js/MediaType::JavaScript files are CJS or MJS. This can be determined a number of ways in node (ex. a package.json having say "type": "module") which is information that's known by the node compat resolvers in the CLI. Perhaps it would be useful to introduce a ModuleType enum that has members ModuleType::Cjs & ModuleType::Mjs and the Resolver.resolve method could return that value which would then be stored in the graph.

@kitsonk
Copy link
Contributor

kitsonk commented Jan 6, 2022

I would prefer something just like a field named kind (type is always a pain in Rust, and actually a bit "wrong" label) and a value like:

enum ModuleKind {
  Amd,
  CommonJs,
  Esm,
  Script,
  SystemJs,
  Umd,
}

While I don't believe swc currently supports AMD/SystemJs/UMD, tsc does, and we should accomodate everything that we likely would be able to understand. That doesn't mean we wouldn't eventually able to do dependency analysis on AMD and SystemJS though. We could easily contribute that dependency analysis up stream though.

The biggest question I have is how this would be used. I assume the use case is to interrogate the graph when loading a module to determine how it should be loaded?

@kitsonk kitsonk added the enhancement New feature or request label Jan 6, 2022
@kitsonk
Copy link
Contributor

kitsonk commented Jan 7, 2022

I was just in the code on another purpose... we would need to consider how the above would related to this, and whether we would merge all of this into a single tagged enum, or restructure it in some way so that EsModule (the existing "code" struct):

#[derive(Debug, Clone, Serialize)]
#[serde(untagged)]
pub enum Module {
  Es(Box<EsModule>),
  Synthetic(Box<SyntheticModule>),
}

All the above would be effectively the same struct as EsModule but just with a tag. Need to think about it a bit more.

@kitsonk
Copy link
Contributor

kitsonk commented Jan 11, 2022

The biggest question I have is how this would be used. I assume the use case is to interrogate the graph when loading a module to determine how it should be loaded?

@bartlomieju and I discussed. It is used in the situation where when loading the module into a runtime, it needs to be interrogated, and for example if an ESM module is loading a CJS module, it needs to go through a process to "convert" the CJS module to an ESM module on the fly.

@bartlomieju
Copy link
Member

This should be named ModuleKind instead of ModuleType as the latter is already used in deno_core.

@kitsonk kitsonk changed the title Potentially have Resolver.resolve return a ModuleType Add ModuleKind to graph and be returned by Resolver.resolve Jan 12, 2022
@kitsonk
Copy link
Contributor

kitsonk commented Jan 14, 2022

I've started working on this... I am going to collapse everything into a single Module struct with a ModuleKind.

kitsonk added a commit to kitsonk/deno_graph that referenced this issue Jan 19, 2022
kitsonk added a commit that referenced this issue Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants