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

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

Open
bjesuiter opened this issue Sep 23, 2022 · 9 comments
Labels
bug Something isn't working correctly node compat

Comments

@bjesuiter
Copy link

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

  • kysely (the query builder)
  • kysely-planetscale (the database adapter for planetscale, uses npm package @planetscale/database internally)

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
Copy link

gabeidx commented Sep 23, 2022

Seems related to #15948.

@bjesuiter
Copy link
Author

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 dsherret added bug Something isn't working correctly node compat labels Sep 23, 2022
@dsherret
Copy link
Member

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 #15823 (comment)

@dsherret dsherret changed the title [node-compat] deno seems to not import transitive dependencies correctly when using a npm: Import [node-compat] deno seems to not import dependency specified in import map from esm package when using a npm: import Sep 23, 2022
@bjesuiter
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

Seems to be the same problem as #15823

@bartlomieju
Copy link
Member

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

@bjesuiter
Copy link
Author

I'll check that, thanks for the ping!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly node compat
Projects
None yet
Development

No branches or pull requests

4 participants