denoland / deno

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

[node-compat] deno seems to not import dependency specified in import map from esm package when using a `npm: ` import #16013

Open bjesuiter opened 1 year ago

bjesuiter commented 1 year ago

I was trying to connect to a planetscale mysql db via the following packages:

I provided all three via an import mapping:

    "kysely": "npm:kysely@0.21.6",
    "kysely/": "npm:kysely@0.21.6/",
    "kysely-planetscale": "npm:kysely-planetscale@0.2.0",
    "kysely-planetscale/": "npm:kysely-planetscale@0.2.0/",
    "@planetscale/database": "npm:@planetscale/database@1.3.0",
    "@planetscale/database/": "npm:@planetscale/database@1.3.0/"

Then I tried to run the following script by using:

deno run --unstable --allow-env=DATABASE_HOST,DATABASE_USERNAME,DATABASE_PASSWORD --allow-net=aws.connect.psdb.cloud  "./scripts/db/minimal-select.ts"

Note: You need to provide the 3 env vars for connecting to a planetscale db: DATABASE_HOST,DATABASE_USERNAME,DATABASE_PASSWORD You can make a demo db for free at: https://auth.planetscale.com/sign-up

// File: ./scripts/db/minimal-select.ts
import { Kysely, sql } from "kysely";
import { PlanetScaleDialect } from "kysely-planetscale";

interface PetsTable {
  id: number;

  name: string;
}

// Keys of this interface are table names.
interface Database {
  pets: PetsTable;
}

export const db: Kysely<Database> = new Kysely<Database>({
  dialect: new PlanetScaleDialect({
    host: Deno.env.get("DATABASE_HOST"),
    username: Deno.env.get("DATABASE_USERNAME"),
    password: Deno.env.get("DATABASE_PASSWORD"),
  }),
});

const query = sql<any>`SHOW TABLES`;
const result = await query.execute(db);
console.log("\n Tables in Planetscale DB: ");
console.log(result);

The Error Message

error: Could not resolve '@planetscale/database' from 'kysely-planetscale@0.2.0'.

The Temporary Fix

I can fix this for now for deno by adding the following import:

// Fixes Runtime Behavior:
import {} from "npm:@planetscale/database@1.3.0";

I think this works because it explicitely loads the @planetscale/database package identifier into the runtime wich makes it resolvable to "kysely-planetscale"

My Expectation

My expectation would be that this simply works without needing my "fake" import.

In theory, deno could resolve all bare specifiers in a package loaded by "npm: " as also being npm packages and load them. If that's not that easy, I would also be ok with adding them to my import_map, as I've already done for my example.

This may be enough to fix this, or do I miss something there?

gabeidx commented 1 year ago

Seems related to https://github.com/denoland/deno/issues/15948.

bjesuiter commented 1 year ago

I looked into it and it seems plausible. The source of kysely-Planetscale is written in TS, but the compile configuration is extended from @tsconfig/node14/tsconfig.json

Which extends to:

{
  "$schema": "https://json.schemastore.org/tsconfig",
  "display": "Node 14",

  "compilerOptions": {
    "lib": ["es2020"],
    "module": "commonjs",
    "target": "es2020",

    "strict": true,
    "esModuleInterop": true,
    "skipLibCheck": true,
    "forceConsistentCasingInFileNames": true,
    "moduleResolution": "node"
  }
}

Links

I will try to use the typescript source directly instead of the compiled package. This should alleviate the problem for me for now. I'll report on the results!

dsherret commented 1 year ago

At the moment, I think packages resolving other packages won't consult the import map, so yeah, I think this is similar to #15948. Also, right now we don't support peer dependencies that well, so you won't get an error/warning here about anything wrong.

Also, we've thought about maybe downloading the packages up front in an import_map, which would solve this problem https://github.com/denoland/deno/issues/15823#issuecomment-1250389394

bjesuiter commented 1 year ago

I don't think that downloading the packages in an import-map in advance would solve the problem completely. I can already cache an npm package locally in the deno cache. This does not get used though with the kysely-planetscale package. (Probably due to the "require" used by the package, as you guys pointed out)

So this requires a fix to the "require" import in the npm dependency, how should downloading the packages in an import map in advance help with that problem?

Also, this sounds like you want to introduce some kind of project local cache, but this sounds basically like node_modules 2.0 for me. Is this the intention with preloading import-map packages or did I misunderstand this?

dsherret commented 1 year ago

kysely-planetscale has an import for the peer dependency @planetscale/database in index.mjs:

import { cast, connect } from "@planetscale/database";

This is specified as a peer dependency in kysely-planetscale's package.json, but on the * version.

What I'm saying is that 1. we need peer dependency support, so it can know about this peer dependency, and 2. that maybe the import map might inform the resolution of what version of @planetscale/database to use. So since you have an entry value for npm:@planetscale/database@1.3.0, perhaps it might know to use 1.3.0 for the resolution based on that instead of the latest version due to the * version specified in the kysel-planetscale's package.json.

Overall, I'm not entirely sure if we should support import maps mapping specifiers in npm packages, but perhaps it would be useful for informing the npm package version resolver about what version to use.

bjesuiter commented 1 year ago

Ahh now I see. I didn't realize that the @planetscale/database was a peer dependency!

Thank you for the explanation!

So that I get it correct: My problem has nothing to do with require imports inside kysely-planetscale coming from the TS build as moduleFormat commonJS?

But instead it comes from the fact that peerDependnecies are currently not resolved?

bartlomieju commented 1 year ago

Seems to be the same problem as https://github.com/denoland/deno/issues/15823

bartlomieju commented 1 year ago

@bjesuiter do you still have this problem with latest Deno version (1.29.1)?

bjesuiter commented 1 year ago

I'll check that, thanks for the ping!