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

Runtime compiler API doesn't require valid specifier #5620

Closed
bartlomieju opened this issue May 19, 2020 · 2 comments
Closed

Runtime compiler API doesn't require valid specifier #5620

bartlomieju opened this issue May 19, 2020 · 2 comments
Labels
cli related to cli/ dir

Comments

@bartlomieju
Copy link
Member

https://doc.deno.land/https/raw.githubusercontent.com/denoland/deno/master/cli/js/lib.deno.unstable.d.ts#Deno.compile

Deno.compile() has an option to take a map of source file. In this option specifiers for files can be written as /foo.ts, /bar.js. It's against other Deno APIs which require valid specifiers and required to add some hackery to make it work:

deno/cli/module_graph.rs

Lines 172 to 181 in 1be7ec4

// FIXME(bartlomieju):
// The resolveModules op only handles fully qualified URLs for referrer.
// However we will have cases where referrer is "/foo.ts". We add this dummy
// prefix "memory://" in order to use resolution logic.
let module_specifier =
if let Ok(spec) = ModuleSpecifier::resolve_url(&specifier) {
spec
} else {
ModuleSpecifier::resolve_url(&format!("memory://{}", specifier))?
};

deno/cli/js/compiler.ts

Lines 605 to 617 in 1be7ec4

mappedUrl = mappedUrl.replace("memory://", "");
SourceFile.cacheResolvedUrl(mappedUrl, importDesc.specifier, entry.url);
}
for (const fileRef of entry.referencedFiles) {
SourceFile.cacheResolvedUrl(
fileRef.resolvedSpecifier.replace("memory://", ""),
fileRef.specifier,
entry.url
);
}
for (const fileRef of entry.libDirectives) {
SourceFile.cacheResolvedUrl(
fileRef.resolvedSpecifier.replace("memory://", ""),

Discussed this situation offline with @ry are there are two potential solutions:

  • require valid specifiers for all files - each specifiers must be a valid URL
  • add baseUrl option to Deno.compile() which will be used to create URLs for each specifier

CC @kitsonk

@bartlomieju bartlomieju added the cli related to cli/ dir label May 19, 2020
@kitsonk
Copy link
Contributor

kitsonk commented May 19, 2020

They are in theory "abstract" resources... requiring them to be them seems quite illogical to me and doesn't sound like good DX. Why even provide the baseUrl option? Again, that is just to keep things "tidy" on the inside, and bad DX to force a user to do something that means nothing to them. I didn't catch it in the PR, but now I understand the problem. It feels like we could more systematically deal with it by just mangling all the sources when they are provided like that without requiring a user to specify a baseUrl and basically tell them "please fake this information because we don't want to".

Also, this particular cleanup might want to wait for #4752 (or we move forward with that) and we would have a discreet API for providing sources, instead of the more conflated one and it maybe easier to resolve the modules in a clean way.

@bartlomieju
Copy link
Member Author

Resolved in #8192

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir
Projects
None yet
Development

No branches or pull requests

2 participants