denoland / deno

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

Cannot use jsr.json as workspace member outside deno publish #24601

Open NfNitLoop opened 2 months ago

NfNitLoop commented 2 months ago
> deno --version
deno 1.45.2 (release, x86_64-pc-windows-msvc)
v8 12.7.224.12
typescript 5.5.2

I found a case where no name seems to be created for a local member.

I was trying to add a dependency as a workspace member to be able to modify/test it locally, a described here.

But, neither of its possible names were made available in the workspace.

Reproduction

mkdir example
cd example
git clone git@github.com:nbd-wtf/nostr-tools.git
# Note, current commit: 54e352d8e2db84070eb12854d8e4705f9fa3d698

Note that the library has a jsr.json with a name of "@nostr/tools", and a package.json with a name of "nostr-tools". I'd expect one of these names to be available when we add this as a workspace member.

// deno.jsonc
{
    "workspace": [ "nostr-tools" ]
}
// main.ts

// Neither of these works:

import { Relay } from "@nostr/tools"
import { Relay } from "nostr-tools"

Results:

error: Relative import path "nostr-tools" not prefixed with / or ./ or ../
    at file:///C:/Users/codyc/code/example/main.ts:2:23

or:

error: Relative import path "@nostr/tools" not prefixed with / or ./ or ../
    at file:///C:/Users/codyc/code/example/main.ts:1:23

I didn't have this problem using this pattern with this library, though: https://github.com/lucacasonato/esbuild_deno_loader

dsherret commented 2 months ago

This is just: https://github.com/denoland/deno/issues/22651

Let's track that there. Also, atm bare specifiers for npm packages aren't supported, but I think we should. Follow https://github.com/denoland/deno/issues/24605 for that

dsherret commented 2 months ago

Oh wait, that one is slightly different

dsherret commented 2 months ago

I've focused this one to it not discovering the jsr.json of the folder (again, let's track the bare specifier for the npm package here: https://github.com/denoland/deno/issues/24605).

I'm not sure we should do this outside of deno publish. It's yet another config file that we need to discover and will make things slower.

NfNitLoop commented 2 months ago

atm bare specifiers for npm packages aren't supported, but I think we should

Agreed. Or, if they're not supported, deno should throw an error/warning when trying to add such a package as a workspace member. Being able to add a thing to a workspace without it getting a name I can use to import it seems broken.

I've focused this one to it not discovering the jsr.json

I'm a bit worried that that's focusing on the solution, rather than the problem. The real problem is that workspaces are documented to work with package.json, I added a library with a package.json, and I got a workspace member that I can't refer to.

I don't mind whether it's fixed by honoring package.json or jsr.json. But I'd very much rather the solution not be "This pattern is unsupported in workspaces" because adding a third-party library as a workspace member (documented here) is a really handy way to work on fixing issues locally, so I'd really like that to work for most third-party libraries I might encounter in the wild. (Whether they use deps.ts, import map, or package.json npm dependencies.)

dsherret commented 2 months ago

I'm a bit worried that that's focusing on the solution, rather than the problem. The real problem is that workspaces are documented to work with package.json, I added a library with a package.json, and I got a workspace member that I can't refer to.

That's what #24605 is. You should be able to refer to it with an npm specifier otherwise or by adding it as a dependency in a package.json (in a package.json, a matching dependency will work or you can use a workspace specifier).

NfNitLoop commented 2 months ago

Was brainstorming about this, and maybe this is a separate ticket for some point in the future, but it would be handy to say how a workspace member is "imported". For example, say I've got a project that uses an import map alias for a dependency:

"imports": {
    "foo": "jsr:@scope/the-great-foo-library"
}

All my code does import foo from "foo"

But now I need to fix that library locally using the pattern I linked to: cloning it locally, and adding it as a workspace member.

"workspace": ["./the_great_foo_library"]

But, now I have to use its declared name, instead of the alias I'm using throughout my codebase. And in the case where it has both jsr.io / package.json, maybe the way I import it doesn't match the (eventual) one that Deno picked to import in that case. Having a way for the workspace to declare the import name here would be handy.

ex, something like:

"workspace": {
    "./the_great_foo_libriary": {
        "importName": "foo",
    }
dsherret commented 2 months ago

But now I need to fix that library locally...

We've been brainstorming this one and we're thinking maybe a separate mechanism for "patching packages" similar to cargo: https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html -- we're going to open an issue soon.

That would handle scenarios like when you want to use a local version of a package, but that package is itself part of another workspace. We'll need a way to resolve with multiple workspaces in that case.