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

A dependency is broken #379

Open
ineu opened this issue Feb 24, 2023 · 15 comments
Open

A dependency is broken #379

ineu opened this issue Feb 24, 2023 · 15 comments

Comments

@ineu
Copy link

ineu commented Feb 24, 2023

https://github.com/eveningkid/denodb/blob/master/deps.ts#L5 uses dex from https://raw.githubusercontent.com/Zhomart/dex/930253915093e1e08d48ec0409b4aee800d8bd0c/mod-dyn.ts, which depends on the latest version of Deno std: https://raw.githubusercontent.com/Zhomart/dex/930253915093e1e08d48ec0409b4aee800d8bd0c/lib-dyn/deps.ts

The latest version doesn't seem to provide node/events anymore. Thus denodb is no longer installable:

error: Module not found "https://deno.land/std/node/events.ts".
    at https://raw.githubusercontent.com/Zhomart/dex/930253915093e1e08d48ec0409b4aee800d8bd0c/lib-dyn/deps.ts:7:24
@cdrn
Copy link

cdrn commented Feb 24, 2023

I've tried replacing it in the import_map.json with https://deno.land/x/events@v1.0.0/mod.ts but haven't had any success - maybe my config is wrong and someone with more deno experience can figure it out from here?

@alum4u
Copy link

alum4u commented Feb 25, 2023

missing node path from std deno module

@jerlam06
Copy link

I created a fix on my fork, just use like this:

export { Database, SQLite3Connector, Model, DataTypes, } from "https://raw.githubusercontent.com/jerlam06/denodb/master/mod.ts";

@mobilifone
Copy link

@jerlam06 Thanks!
Dario

@hugopeixoto
Copy link

No need to use a fork, I was able to patch the imports by adding the following to import_map.json, removing deno.lock, and running again:

{
  "imports": {
    // stuff that was already there
    "https://deno.land/std/node/util.ts": "node:util",
    "https://deno.land/std/node/events.ts": "node:events",
    "https://deno.land/std/node/assert.ts": "node:assert",
    "https://deno.land/std/node/url.ts": "node:url",
    "https://deno.land/std/node/stream.ts": "node:stream"
  }
}

@redice
Copy link

redice commented Mar 8, 2023

I overrided imports in import_map.json and that works well.

{
  "imports": {
   ......
    "denodb": "https://deno.land/x/denodb@v1.2.0/mod.ts",
    "https://deno.land/std/": "https://deno.land/std@0.177.0/"
  }
}

@eveningkid
Copy link
Owner

Hey everyone,

Thanks a lot for looking into this. I'm not active on this repo hence the delay!

Is the import_map our only way to go?
Or can we use a different version of dex somewhere to not lose the node/events part?

If someone has a fix outside of import_map, I'd be happy to quickly review a PR and get this merged :)

Just let me know

@hugopeixoto
Copy link

After investigating this further, I think we have two options:

Option 1: Go back to using the original https://deno.land/x/dex/mod.ts instead of Zhomart's fork. The original rationale for the fork was:

dex: update to the version that uses std@0.115.1, instead of the latest version of jspm-core, which could be dangerous when jspm-core pushes the latest braking code.

This doesn't seem true, because dex/mod.ts vendors all its dependencies. There are two dex versions: mod.js and mod-dyn.js. The dyn version uses "live" dependencies, so in that specific version there is the risk that Zhomart mentions. But mod.js vendors everything, so even if jspm-core updates their code, it won't affect dex. Not sure if this is strictly true, since this change must have been done after hitting some problem. The downside of this approach is that we won't get bugfixes to their dependencies, since they're vendored.

Option 2: Keep using mod-dyn, but use another fork that keeps these dependencies up to date. There's a PR on Zhomart's fork that pins the dependencies to std/node@0.177.0, but I think we should use node:* instead, so I created a PR on the original dex that makes that change: aghussb/dex#5

@eveningkid
Copy link
Owner

Awesome, thanks @hugopeixoto for all these details!

Let's go with option 2, and if the pr does not get merged, we can directly use your fork too

@nberlette
Copy link

nberlette commented Mar 14, 2023

I did a simple fix using the scopes property of the import map syntax. Since Deno now supports imports and scopes properties directly in the deno.json file, I created a deno.json file in the project root and added this to it:

{
  // tasks, etc...
  "scopes": {
    "https://raw.githubusercontent.com/Zhomart/dex/": {
      "https://deno.land/std/": "https://deno.land/std@0.177.0/"
    }
  }
}

Immediately fixed the issue for me.

Note: that the dependency in question is relying on the node modules of the Deno Standard Library, which were removed in 0.178.0. So I have the scope pointing to 0.177.0, the last version in which they were present.

@nberlette
Copy link

Sorry for being a bit late to the party, but I agree that option 2 from above seems the best candidate. In the interim, the patch in my previous comment provides a quick and immediate fix. It also adds a badge to deno.land/x/denodb that says Includes Deno Configuration 😉

The scopes capability is very powerful, but unfortunately is lacking in documentation; many people just don't know what it does. I've deployed it as a hotfix for similar broken dependency issues on several occasions now.

@pvillaverde
Copy link

I did a simple fix using the scopes property of the import map syntax. Since Deno now supports imports and scopes properties directly in the deno.json file, I created a deno.json file in the project root and added this to it:

{
  // tasks, etc...
  "scopes": {
    "https://raw.githubusercontent.com/Zhomart/dex/": {
      "https://deno.land/std/": "https://deno.land/std@0.177.0/"
    }
  }
}

Immediately fixed the issue for me.

Note: that the dependency in question is relying on the node modules of the Deno Standard Library, which were removed in 0.178.0. So I have the scope pointing to 0.177.0, the last version in which they were present.

hey there! I'm trying to apply this scopes as a temporary fix but when I try to build the docker image with deno app I still got the dependency broken...

error: Module not found "https://deno.land/std/node/events.ts".
    at https://raw.githubusercontent.com/Zhomart/dex/930253915093e1e08d48ec0409b4aee800d8bd0c/lib-dyn/deps.ts:7:24
The command '/bin/sh -c deno cache ./src/deps.ts' returned a non-zero code: 1

Do I need to do any other change besides the scopes on deno.json? Thanks!

@nberlette
Copy link

@pvillaverde Sorry for the late response; hopefully you've got this figured out by now. I'm not too familiar with using Docker + Deno unfortunately, but have you tried applying the scopes in an import_map.json instead?

Using the imports and scopes property directly in deno.json is a relatively new feature. Not knowing what version of Deno you're running, I'd say it might be plausible to try an import map instead (which have been supported since like, day 1).

You can force the command to use the import map like so:

/bin/sh -c deno cache --import-map=./import_map.json ./src/deps.ts

Let me know how it goes.

Reflecting on this issue, this is yet another example of why everyone needs to use pinned versions of their dependencies. We never know when a breaking change (like Deno suddenly dropping std/node in 0.178.0) will happen. But that one developer's choice to take the lazy route results in all of our projects being reduced to nothing more than an uncaught exception.

@pvillaverde
Copy link

@nberlette Thanks for answering! Dont worry for the late response, I have it working with the current version and just couldn't update it with some new features.

I'm using the latest deno:alpine image as base, so it should work with the scopes properties on deno.json. However with the import_map.json it worked! I had tried it before but didn't realise that on the DockerFile when I run de "deno cache" I haven't copied all the files yet, just the deps.ts. Once I copied the import_map.json it build all the dependencies.

Thanks!!

@nberlette
Copy link

@nberlette Thanks for answering! Dont worry for the late response, I have it working with the current version and just couldn't update it with some new features.

I'm using the latest deno:alpine image as base, so it should work with the scopes properties on deno.json. However with the import_map.json it worked! I had tried it before but didn't realise that on the DockerFile when I run de "deno cache" I haven't copied all the files yet, just the deps.ts. Once I copied the import_map.json it build all the dependencies.

Thanks!!

Sweet, I'm glad to hear it worked for you.

Take care brotha!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants