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

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

Closed
dsherret opened this issue Jan 22, 2023 · 13 comments
Closed

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

dsherret opened this issue Jan 22, 2023 · 13 comments

Comments

@dsherret
Copy link
Member

dsherret commented Jan 22, 2023

Extracted from denoland/deno#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:

  • /x/module/mod.ts
  • /x/module/mod.js
  • /x/module/index.ts
  • /x/module/index.js
  • /x/module/src/mod.ts
  • /x/module/src/mod.js

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

@albnnc
Copy link

albnnc commented Jan 22, 2023

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

dsherret commented Jan 22, 2023

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

aapoalas commented Jan 23, 2023

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

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

ry commented Jan 24, 2023

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

dsherret commented Jan 24, 2023

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

aapoalas commented Jan 24, 2023

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

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

KnorpelSenf commented Jan 24, 2023

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

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

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

retraigo commented Feb 26, 2023

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.

@crowlKats crowlKats transferred this issue from denoland/dotland May 31, 2023
@dsherret
Copy link
Member Author

This will not be done.

@dsherret dsherret closed this as not planned Won't fix, can't repro, duplicate, stale Nov 10, 2023
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

7 participants