denoland / website_feedback

For reporting issues & suggestions for deno.com and deno.land
9 stars 1 forks source link

Redirect module path to exact mod.ts/js index.ts/js file #10

Closed dsherret closed 10 months ago

dsherret commented 1 year ago

Extracted from https://github.com/denoland/deno/issues/17475

Take notice that this is in the dotland repo and is not a change to Deno's module resolution. This is a proposal to create a new endpoint on the registry that does a redirect.

/x/module redirects to whichever of these exist, ordered by precedence:

This allows people do leave out the path when importing modules (ex. import { isOdd } from "deno:is_odd")

albnnc commented 1 year ago

Continuing roadmap discussion, it is important to understand the reasoning for this change.

Also, is this going to be a particular feature of /x/, or a general recommendation for Deno-related CDNs?

EDIT

My bad, I've just seen that this a dotland repo. A think that it's quite clear what the answer for the last question is. If I'm wrong, please, take a note on that.

dsherret commented 1 year ago

It's not necessarily a recommendation, just an added feature of the deno.land/x registry so people can specify more terse specifiers.

Personally, I don't think we shouldn't do index.ts/index.js redirects and instead only have mod.ts/mod.js ones for consistency.

aapoalas commented 1 year ago

It seems like a fairly odd choice to provide this sort of magic "index file" functionality, especially when it has an order of precedence and even peeks inside essentially "random" folders like src for a possible index file.

/x/ already supports deno.json detection, so would it make more sense to copy the main file definition from package.json to align with prior art and avoid repeating old spells?

EDIT: Adding a main file definition to deno.json would give further expectations that any folder import with deno.json in it would be automatically resolved by Deno using the main field. This places a bad precedent and expectation on users

KnorpelSenf commented 1 year ago

I don't think we should do this at all. What's the point of this? Saving like 7 keystrokes (which are auto-completed anyway)?

I do not think that being ambiguous or needlessly implicit is a goal. Magically finding the right file in a directory is as good as dropping file extensions. I wonder what @ry thinks about this?

ry commented 1 year ago

I came up with this. It's important to note that this is a deno.land/x feature, not a CLI module system change. It's to save keystrokes.

dsherret commented 1 year ago

I don't think this would be considered magic since /x/module would say what it redirects to. This functionality would be a feature of the /x/module endpoint.

aapoalas commented 1 year ago

A plain redirect wouldn't itself be particularly magical but the resolving and precedence of it kind of is. Do you prefer mod.ts and mod.js? In that case you're choosing the entry point for modules / libraries published into /x/ for them and quite clearly establishing a precedent. You're even taking a stand against the majority of what JavaScript world has gotten used to, since for more than 10 years the basis has been Ry's previous choice of index.js.

The redirect might not even work:

import foo from "https://deno.land/x/foo";
import bar from "https://deno.land/x/bar";

Does the above foo import work? Who knows! It depends entirely on if the foo library happens to have a mod.ts or not. These two equivalent imports might have one of them work and the other error out. Furthermore, although a somewhat theoretical exercise, it's possible that foo would indeed have a mod.ts file but it's not actually the intended entry point and instead f.ex. a foo.ts file is to be used.

Or perhaps you choose to resolve in order to one of (leaving out TS/JS changes) mod.ts, index.ts, and src/mod.ts. You're now increasing the chance that the wrong file is chosen accidentally. Perhaps a library provides index.js for browser imports and a lib.ts for usage in Deno. The magic will break edge cases.

Furthermore, this automatic redirection especially if paired with the deno: imports (which for the record I believe to be a bad idea) will also set a precedent for folders to be importable. eg. If I checkout or vendor a library that is published on /x/ I might have the expectation that I can save keystrokes and import it from "./vendor/foo" and will be disappointed when this does not work like it works with /x/. This can of course be done using import maps, but a first-comer or even an experienced Node.js developer won't know that and will probably open a GitHub issue about folder resolving being broken.

Deno is also aiming to support loading of NPM modules / Node.js code from local file system. Those will resolve their imports through index.js first, or whatever package.json says if present. There will then be four... five? different ways an import can resolve depending on the context:

// In Node..js compat code:
import "./folder"; // resolves to "./folder/index.js" or "./folder.js"
import "library"; // resolves to NPM "library", or to package.json-relative "library.js" or "library/index.js"

// In Deno code
import "https://deno.land/x/lib/entry.ts";
import "https://deno.land/x/lib"; // resolves to "lib/mod.ts" or "lib/index.ts" or "lib/src/index.ts", or fails if none of those exist
import "deno:lib"; // Same as above
import "./folder"; // Likely an error; Node.js compat mode imports cannot be copied over.
import "./folder/index.js"; // Proper import of above.
import "./vendor/lib"; // Above `deno:lib` vendored locally: This now doesn't work, the imports aren't "translatable" from remote to local.

NPM of course does this sort of automatic "redirection". I expect it originally simply used index.js but nowadays the main file definition exists, as well as browser. Above I suggested using a similar approach for /x/ but I had to take that back after some thought: Including a main field introduces the expectation that a folder with a deno..json will resolve using the main file definition, just like it would do on /x/. This sort of /x/ registry metadata field would also start binding Deno and the /x/ registry tighter together, repeating the same mistake of Node and NPM being so tightly bound as to be essentially one.

If I had to pick and choose one way to go about this, I would introduce some x.json metadata file meant for publishing to /x/ only. That file could then contain a main field that /x/ could resolve to. This would avoid both the edge cases and binding Deno and /x/ tightly together through the deno.json.

ayame113 commented 1 year ago

I'm curious if you're trying to redirect only the ts and js extensions, or the other extensions (jsx, tsx, cjs, cts, mjs, mts, d.ts) as well. In the latter case, what is the order of precedence for redirects?

KnorpelSenf commented 1 year ago

Currently, the LSP doesn't like redirects. It spits out a warning and tells us to directly use the correct file to import, rather than relying on the remote logic to do so. Would this change then receive an exception? The LSP somehow needs to know about the module resolution logic of /x and then avoid suggesting to use the target of the redirect.

I have an alternative suggestion. If we want to save keystrokes, why not leave the registry the way it is and add better auto-complete logic to the LSP? It could easily check for the existence of the remote files mod.ts, mod.js, index.ts, and so on, and then suggest to use these files. What do you think about this?

dsherret commented 1 year ago

Below are my personal opinions.

The redirect might not even work

That's ok. It's up to a module author whether they would like to take advantage of this endpoint or not. It's true though that they might accidentally take advantage of this when they don't want to, so that's why I think it should only be for mod.ts/js since that's what's mostly used on the registry already. It would help drive some standardization.

These two equivalent imports might have one of them work and the other error out.

This is similar to https://deno.land/x/foo/mod.ts being resolved and https://deno.land/x/bar/mod.ts not being resolved.

Perhaps a library provides index.js for browser imports and a lib.ts for usage in Deno. The magic will break edge cases.

The folder endpoint would only resolve that way for the Deno user agent from my understanding, so people could use a file that's more explicit for browsers.

NPM of course does this sort of automatic "redirection".

That's part of node's resolution and not a feature of the registry. This case is fundamentally different because it doesn't change Deno's module resolution.

I'm curious if you're trying to redirect only the ts and js extensions, or the other extensions (jsx, tsx, cjs, cts, mjs, mts, d.ts) as well. In the latter case, what is the order of precedence for redirects?

Most likely just .ts and .js.

Currently, the LSP doesn't like redirects. It spits out a warning and tells us to directly use the correct file to import, rather than relying on the remote logic to do so. Would this change then receive an exception? The LSP somehow needs to know about the module resolution logic of /x and then avoid suggesting to use the target of the redirect.

My personal opinion is the LSP should still offer this quick fix.

I have an alternative suggestion. If we want to save keystrokes, why not leave the registry the way it is and add better auto-complete logic to the LSP? It could easily check for the existence of the remote files mod.ts, mod.js, index.ts, and so on, and then suggest to use these files. What do you think about this?

This should probably be done anyway.

KnorpelSenf commented 1 year ago

Currently, the LSP doesn't like redirects. It spits out a warning and tells us to directly use the correct file to import, rather than relying on the remote logic to do so. Would this change then receive an exception? The LSP somehow needs to know about the module resolution logic of /x and then avoid suggesting to use the target of the redirect.

My personal opinion is the LSP should still offer this quick fix.

In other words, this issue suggests to implement a feature which the LSP in turn discourages to use.

retraigo commented 1 year ago

It would help drive some standardization.

Was it not Deno's intention to not standardize anything just for Deno? Like not following Node's footsteps.

dsherret commented 10 months ago

This will not be done.