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

feat: Import maps #2360

Merged
merged 56 commits into from Jun 9, 2019

Conversation

3 participants
@bartlomieju
Copy link
Contributor

commented May 15, 2019

This PR aims to add support for import maps as described in #1921

For now code is rather poor quality, I intend to correct that in the next days.

It's still very early stage, things to be done:

  • reading import map from JSON (waiting #2359)
  • scopes

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-import_maps branch from f10b9bf to cbf35ad May 16, 2019

Show resolved Hide resolved cli/import_map.rs Outdated

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-import_maps branch from abb0eb6 to cdc15a1 May 16, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@ry please take a look. Got parsing working (there is still some cases with std:* to handle) and most of resolving, now I'd like to start integrating import maps, a natural place for me to use them would be:

impl Loader for Worker {
  type Error = DenoError;

  fn resolve(specifier: &str, referrer: &str) -> Result<String, Self::Error> {
    if import_map {
      let result = import_map.resolve(specifier, referrer);
      ...
    }
    resolve_module_spec(specifier, referrer).map_err(DenoError::from)
  }

  ...
}

But it's not straight forward as Worker implements Loader that lives in core/modules.rs. Any tips?

@bartlomieju bartlomieju marked this pull request as ready for review May 16, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-import_maps branch 2 times, most recently from 5c8ba86 to ffc298d May 17, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Okay this is more-or-less ready for some review.

cli/import_map.rs is basically one-to-one implementation of reference implementation. There are two differences:

  • no builtin modules - we don't support them, so one can't remap specifier to builtin module
{
 // this will not work
  "imports": {
     "foo": ["std:blank"]
  }
}
  • not all fetch schemes are supported - Deno supports only reading from http:, https: and file:
{
 // this will not work
  "imports": {
     "foo": ["data:<asdfasdf>"],
     "bar": ["blob:<asdfasdf>"],
  }
}

Now it's time to integrate this remapping to module loading flow. @ry I will definitely need some help with that

EDIT: Regarding use of serde_derive; I'm pretty happy with current manual parsing of JSON - it's pretty straightforward and readable, after all I think it's not worth to use it in this case.

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

I tried again to integrate import map resolving inside core/modules.rs - this can basically be done changing signature of Loader::resolve, here's "almost" working diff (at the end loader is taken and cannot be used in resolve_cb):

--- a/core/modules.rs
+++ b/core/modules.rs
@@ -41,7 +41,12 @@ pub trait Loader {
   /// When implementing an spec-complaint VM, this should be exactly the
   /// algorithm described here:
   /// https://html.spec.whatwg.org/multipage/webappapis.html#resolve-a-module-specifier
-  fn resolve(specifier: &str, referrer: &str) -> Result<String, Self::Error>;
+  fn resolve(
+    &self,
+    specifier: &str,
+    referrer: &str,
+    is_root: bool,
+  ) -> Result<String, Self::Error>;

   /// Given an absolute url, load its source code.
   fn load(&mut self, url: &str) -> Box<SourceCodeInfoFuture<Self::Error>>;
@@ -105,7 +110,8 @@ impl<L: Loader> RecursiveLoad<L> {
     referrer: &str,
     parent_id: Option<deno_mod>,
   ) -> Result<String, L::Error> {
-    let url = L::resolve(specifier, referrer)?;
+    let loader = self.loader.as_mut().unwrap();
+    let url = L::resolve(&loader, specifier, referrer, true)?;

     let is_root = if let Some(parent_id) = parent_id {
       let loader = self.loader.as_mut().unwrap();
@@ -261,13 +267,14 @@ impl<L: Loader> Future for RecursiveLoad<L> {
     }

     let root_id = self.root_id.unwrap();
+    let imm_loader = &self.loader.unwrap();
     let mut loader = self.take_loader();
     let (isolate, modules) = loader.isolate_and_modules();
     let result = {
       let mut resolve_cb =
         |specifier: &str, referrer_id: deno_mod| -> deno_mod {
           let referrer = modules.get_name(referrer_id).unwrap();
-          match L::resolve(specifier, &referrer) {
+          match L::resolve(imm_loader, specifier, &referrer, false) { // this bit is not working 
             Ok(url) => match modules.get_id(&url) {
               Some(id) => id,
               None => 0,
@@ -634,7 +641,12 @@ mod tests {
   impl Loader for MockLoader {
     type Error = MockError;

-    fn resolve(specifier: &str, referrer: &str) -> Result<String, Self::Error> {
+    fn resolve(
+      &self,
+      specifier: &str,
+      referrer: &str,
+      _is_root: bool,
+    ) -> Result<String, Self::Error> {
       eprintln!(">> RESOLVING, S: {}, R: {}", specifier, referrer);
       let output_specifier =
         if specifier.starts_with("./") && referrer.starts_with("./") {
@ry
Copy link
Collaborator

left a comment

It's a good start - nice work.
I'm most concerned about Value types propagating beyond from_json and the use of IndexMap.

Show resolved Hide resolved cli/import_map.rs Outdated
Show resolved Hide resolved cli/import_map.rs Outdated
Show resolved Hide resolved cli/import_map.rs Outdated
Show resolved Hide resolved cli/import_map.rs Outdated
Show resolved Hide resolved cli/import_map.rs Outdated
Show resolved Hide resolved cli/import_map.rs Outdated
Show resolved Hide resolved cli/import_map.rs Outdated
Show resolved Hide resolved cli/import_map.rs Outdated
Show resolved Hide resolved cli/state.rs Outdated
Show resolved Hide resolved cli/state.rs
@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@ry I've updated the PR according to your feedback, please take a look at not-resolved comments to settle on the rest of them. Also if you could suggest something with respect to #2360 (comment) I'd be very happy

@ry

This comment has been minimized.

Copy link
Collaborator

commented May 20, 2019

this can basically be done changing signature of Loader::resolve

I'm ok with adding the &self parameter, but why do you need is_root ?

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

this can basically be done changing signature of Loader::resolve

I'm ok with adding the &self parameter, but why do you need is_root ?

Yes, is_root is needed - import map shouldn't be applied to main module or web worker's main module, so a check if needed in resolve.

I'm having trouble making borrow checker work in RecursiveLoad::poll() - Loader::resolve is called inside callback to isolate.mod_instantiate(root_id, &mut resolve_cb), could you check patch above?

Show resolved Hide resolved core/modules.rs Outdated
RecursiveLoad shouldn't own the Isolate
This patch makes it so that RecursiveLoad doesn't own the Isolate, so
Worker::execute_mod_async does not consume itself.

Previously Worker implemented Loader, but now ThreadSafeState does.

This is necessary preparation work for dynamic import (#1789) and import
maps (#1921)

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-import_maps branch from 238812f to 051ceaf Jun 5, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-import_maps branch from fe80eb2 to 145cb26 Jun 7, 2019

@ry ry referenced this pull request Jun 7, 2019

Open

Major features necessary for 1.0 #2473

5 of 21 tasks complete
@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Working example:

// importmap.json
{
  "imports": {
    "moment": "./moment.ts",
    "lodash": ["./lodash.ts"]
  }
}

// moment.ts
console.log("I'm remapped moment");

// lodash.ts
console.log("I'm remapped lodash");

// script.ts
import "moment";
import "lodash";
console.log("Import maps in action!");
$ deno --importmap=importmap.json script.ts
I'm remapped moment
I'm remapped lodash
Import maps in action! 
@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 7, 2019

A better example would be to map "http" to "https://deno.land/std/http/" , so that it looks like

import { serve } from "http/server.ts";

async function main() {
  const body = new TextEncoder().encode("Hello World\n");
  for await (const req of serve(":8000")) {
    req.respond({ body });
  }
}

main();

Would be a nice one for the manual.

bartlomieju added some commits Jun 8, 2019

fmt

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-import_maps branch from 3defdf9 to f19d9c9 Jun 8, 2019

Arg::with_name("importmap")
.long("importmap")
.value_name("FILE")
.help("Load import map file")

This comment has been minimized.

Copy link
@ry

ry Jun 8, 2019

Collaborator

Can you link to the spec, gist, or manual so people can learn more about the file format?

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jun 9, 2019

Author Contributor
--importmap <FILE>
    Load import map file
    Specification: https://wicg.github.io/import-maps/
    Examples: https://github.com/WICG/import-maps#the-import-map

Caveat - visible only with deno --help

}
}

pub fn from_json(

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Jun 8, 2019

Collaborator

I wonder if it would be easier to use serde_derive to automatically generate a parser for this.
Might be a lot less code and reduce risk of bugs.

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jun 9, 2019

Author Contributor

I went that way, but after several passes I decided that it's actually easier and less code to do it manually (coercing different types to array turned out to be a lot of code in serde_derive). Test suite is pretty big, so this shouldn't be a problem

@@ -498,10 +499,30 @@ fn op_fetch_module_meta_data(
let use_cache = !state.flags.reload;
let no_fetch = state.flags.no_fetch;

// TODO(bartlomieju): I feel this is wrong - specifier is only resolved if there's an

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Jun 8, 2019

Collaborator

What does the spec say about modules that don't appear in the import map?
Is there some default behavior to fall back to?

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jun 9, 2019

Author Contributor

Yes, it's fall-through - in our case ImportMap::resolve returns None and specifier is resolved as if there was no import map

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jun 9, 2019

Author Contributor

But my comment concerns whole op_fetch_module_meta_data - I believe that there should be resolving of specifier even if there is no import map - to see that, please tak a look at tests/error_004_missing_module.ts. It not returning proper error. More for reference in this comment

Show resolved Hide resolved website/manual.md Outdated

@bartlomieju bartlomieju referenced this pull request Jun 9, 2019

Open

Rewrite deno_dir.rs #2057

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

@ry @piscisaureus feedback addressed

@ry

ry approved these changes Jun 9, 2019

Copy link
Collaborator

left a comment

LGTM - awesome work!

I think we’ll still need to iterate on this, but let’s do it in master and not have you keep rebasing.

@ry ry merged commit a115340 into denoland:master Jun 9, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

🎉 Thanks! Great to have it landed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.