Skip to content

Commit

Permalink
Fix incorrect scope by the cli script (close #480)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ije
ije committed Jan 12, 2023
1 parent 77ae2d3 commit 7c4cc77
Showing 1 changed file with 4 additions and 11 deletions.
15 changes: 4 additions & 11 deletions server/embed/deno_cli.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,24 +329,17 @@ async function addPkgToImportMap(
importMap.imports[aliasName + "/"] = pkgUrl + "/";
}
if (pkg.dependencies) {
if (!pkg.subModule) {
importMap.scopes[aliasName] = {};
}
if (withExports || pkg.subModule) {
importMap.scopes[pkg.name + "/"] = {};
const esmshScope = `https://esm.sh/${VERSION}/`;
if (!Reflect.has(importMap.scopes, esmshScope)) {
importMap.scopes[esmshScope] = {};
}
for (const [depName, depVersion] of Object.entries(pkg.dependencies)) {
const dep = `${depName}@${depVersion}`;
const depPkg = await fetchPkgInfo(dep);
if (depPkg) {
const depUrl =
`${importUrl.origin}/${VERSION}/${depPkg.name}@${depPkg.version}`;
if (!pkg.subModule) {
importMap.scopes[aliasName][depName] = depUrl;
}
if (withExports || pkg.subModule) {
importMap.scopes[pkg.name + "/"][depName] = depUrl;
}
importMap.scopes[esmshScope][depName] = depUrl;
}
}
}
Expand Down

3 comments on commit 7c4cc77

@lecstor
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A single scope for esm.sh has high potential to break things though doesn't it?

ie, if a project has two dependencies that in turn depend on different versions of the same package, then one will get the wrong version.

If not, then what is the point of using scopes, why not just add them to imports?

If there is a single scope for esm.sh, then I think you're headed into a dependency resolution minefield where npm, yarn, pnpm spend a lot of there time I think.

(very new to Deno so I quite well might be missing some context here)

@ije
Copy link
Member Author

@ije ije commented on 7c4cc77 Jan 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lecstor i agree with you, the right import maps with version conflict should like:

{
      "imports": {
        "react-dom": "http://localhost:8080/v100/*[email protected]",
        "swr": "http://localhost:8080/v100/*[email protected]&deps=react@17",
        "swr/": "http://localhost:8080/v100/*[email protected]&deps=react@17/"
      },
      "scopes": {
        "http://localhost:8080/v100/": {
          "react": "http://localhost:8080/v100/[email protected]"
        },
        "http://localhost:8080/v100/[email protected]/deno/swr.js": {
          "react": "http://localhost:8080/v100/[email protected]"
        }
      }
    }

@lecstor
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that example you have me wondering about how Deno deals with peerDependencies as having different versions of React will likely lead to context and hooks issues so then you do only want a single version..

ok, I get the feeling this is not a solved (or recognised) problem in Deno.. denoland/deno#15950

Please sign in to comment.