feat: include some possible dynamic imports with template literals in graph #334
Conversation
dsherret
requested review from
marvinhagemeister,
lucacasonato and
bartlomieju
| // TODO(nayeemrmn): Import attributes should be visited and checked for | ||
| // every import, not one per specifier. | ||
| if dep.maybe_attribute_type.is_none() { | ||
| dep.maybe_attribute_type = import.attributes.get("type").cloned(); | ||
| } |
| maybe_resolver: Option<&dyn Resolver>, | ||
| maybe_module_analyzer: Option<&dyn ModuleAnalyzer>, | ||
| maybe_npm_resolver: Option<&dyn NpmResolver>, | ||
| options: ParseModuleOptions, |
There was a problem hiding this comment.
Nice to see this cleaned up!
| run_test( | ||
| " | ||
| await import(`./${test}`); | ||
| ", | ||
| vec![ | ||
| ("file:///a/mod.ts", ""), | ||
| ("file:///a/sub_dir/a.ts", ""), | ||
| ("file:///b.ts", ""), | ||
| ], | ||
| vec!["file:///a/mod.ts", "file:///a/sub_dir/a.ts", "file:///b.ts"], | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
To be honest I don't understand this bit - I assume you are returning all files that "might" match depending on the value of test variable - will this be handled somehow downstream or is the goal to just discover all possible files that might get imported?
There was a problem hiding this comment.
I think it works like in vite where it treats variables in the string as a ** glob so it would include any possible file.
There was a problem hiding this comment.
I believe it uses a * glob instead of a ** glob.
For reference, the Vite documentation notes that variables only represent file names one level deep.
Additionally, Vite utilizes @rollup/plugin-dynamic-import-vars, which is described here. According to the documentation:
When generating globs, each variable in the string is converted to a glob * with a maximum of one star per directory depth. This avoids unintentionally adding files from many directories to your import.
There was a problem hiding this comment.
for example:
// /dev/main.ts
await import(`./b/${test}.ts`)This should not include: file:///dev/b/c/c/test.ts. However, currently, it does include that path.
There was a problem hiding this comment.
We ended up going with this behaviour here that's similar to vite, but not exactly.
Closes #275