denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
93.81k stars 5.22k forks source link

cli: import map support for `deno add` #24264

Closed scarf005 closed 1 week ago

scarf005 commented 2 months ago

Status Quo

using deno add will break existing importMap section in deno.json.

$ deno add @std/fs
// deno.json
{
-  "importMap": "import_map.json"
+  "importMap": "import_map.json",
+  "imports: {
+    "@std/fs": "jsr:@std/fs@^0.229.3",
+  }
}

Suggestion

if importMap exists, attempt to update import map instead.

// import_map.json
{
  "imports: {
+    "@std/fs": "jsr:@std/fs@^0.229.3",
  }
}

Others

I'm interested in opening a PR (pointers on where to look at would be appreciated!)

HasanAlrimawi commented 1 month ago

@lucacasonato , So the program should check if there's an importMap file, and if yes then it should insert the added dependency there and ignore adding it into the deno.json even if it (the deno.json file) had the "imports" key?

lucacasonato commented 1 month ago

I'm not sure we should implement this feature. But if we did do it, yes that is how I imagine it would work.

scarf005 commented 1 month ago

I'm not sure we should implement this feature.

could you explain why? because imports section is non-standard whereas import map is browser standard.

HasanAlrimawi commented 1 month ago

I'm not sure we should implement this feature. But if we did do it, yes that is how I imagine it would work.

If it should be, then I'm interested in contributing to the implementation or sharing my insights with @scarf005 on an approach I have in mind to implement this.

dsherret commented 1 week ago

I agree with Luca on not adding this one. It's extra work to maintain and I don't believe it's worth that extra work. Import maps do not use the extended import map found in a deno.json. For example, if you run deno add jsr:@std/fs then it should probably generate an import map that looks like:

{
  "imports": {
+    "@std/fs": "jsr:@std/fs@^0.229.3",
+    "@std/fs/": "jsr:/@std/fs@^0.229.3/"
  }
}

If someone is using jsr or npm specifiers, then they should probably be using a deno.json and most of the value of deno add is found with jsr and npm specifiers.

Considering that the "imports" field takes precedence over the "importMap" field, I think we should instead error when the deno.json contains an "importMap" field when using deno add.