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

suggestion: simplify internal module specifiers #24294

Open
iuioiua opened this issue Jun 20, 2024 · 4 comments
Open

suggestion: simplify internal module specifiers #24294

iuioiua opened this issue Jun 20, 2024 · 4 comments
Labels
deno_core Changes in "deno_core" crate are needed suggestion suggestions for new features (yet to be agreed)

Comments

@iuioiua
Copy link
Collaborator

iuioiua commented Jun 20, 2024

The internal import map for this codebase is defined in tools/core_import_map.json. Currently, each import corresponds to an individual script. This means a new entry is needed for every new script, and when one entry is missing, VSCode IntelliSense doesn't work for symbols from that script.

I suggest replacing these single-script imports with directory imports and changing the prefix used in internal module specifiers. This change would automatically cover almost all internal imports with near-zero maintenance. @bartlomieju told me this would require considerable changes to this repo and deno_core code and, therefore, likely best done after Deno 2.

Please excuse me if I've overlooked some obvious drawbacks to this approach. I have little context on why internal module specifiers were done this way in the first place.

Currently

"ext:deno_node/_utils.ts": "../ext/node/polyfills/_utils.ts",
"ext:deno_node/_zlib_binding.mjs": "../ext/node/polyfills/_zlib_binding.mjs",
"ext:deno_node/00_globals.js": "../ext/node/polyfills/00_globals.js",
"node:module": "../ext/node/polyfills/01_require.js",
"node:assert": "../ext/node/polyfills/assert.ts",
"node:assert/strict": "../ext/node/polyfills/assert/strict.ts",

import { zlib as constants } from "ext:deno_node/internal_binding/constants.ts";
import { TextEncoder } from "ext:deno_web/08_text_encoding.js";
import { Transform } from "node:stream";
import { Buffer } from "node:buffer";

Suggestion

// tools/core_import_map.json
{
  "imports": {
    "ext/": "../ext/",
    "ext/node/": "../ext/node/polyfills",
    "ext/runtime/": "../runtime/js/",
    "node:assert": "../ext/node/polyfills/assert.ts",
    "node:assert/strict": "../ext/node/polyfills/assert/strict.ts"
    // ...
  }
}
// ext/node/polyfills/_brotli.js
// ...
import { zlib as constants } from "ext/node/internal_binding/constants.ts";
import { TextEncoder } from "ext/web/08_text_encoding.js";
import { Transform } from "node:stream";
import { Buffer } from "node:buffer";
// ...
@iuioiua iuioiua added deno_core Changes in "deno_core" crate are needed suggestion suggestions for new features (yet to be agreed) labels Jun 20, 2024
@lucacasonato
Copy link
Member

I don't think this would require deno_core changes if the only thing you want to change is the import map.

@iuioiua
Copy link
Collaborator Author

iuioiua commented Jun 21, 2024

We'd have to change the import map as well as module specifiers in TS/JS files.

@lucacasonato
Copy link
Member

Oh I see - we could just change the specifiers to ext:/... - deno_core could likely support that easially

@iuioiua
Copy link
Collaborator Author

iuioiua commented Jun 22, 2024

I tried that, and IntelliSense didn't work (see here). Perhaps I'm missing something obvious, but I can't find documentation on directory-based imports with prefixes like ext:.

Ref #17188

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deno_core Changes in "deno_core" crate are needed suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

No branches or pull requests

2 participants