denoland / deno

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

Module resolution doesn't work correctly when the module specifier includes double slashes `//` #25532

Open janispritzkau opened 3 weeks ago

janispritzkau commented 3 weeks ago

Version: Deno 1.46.3

Deno REPL:

> import "npm:hono/streaming"
Uncaught TypeError: [ERR_MODULE_NOT_FOUND] Cannot find module 'file:///Users/x/Library/Caches/deno/npm/registry.npmjs.org/hono/4.5.11/dist/helper/utils/stream.js' imported from 'file:///Users/x/Library/Caches/deno/npm/registry.npmjs.org/hono/4.5.11/dist/helper/streaming//stream.js'
    at async <anonymous>:1:22

I'm not quite sure how this happens. I notice that there are two slashes in the "imported from" module.

janispritzkau commented 3 weeks ago

It looks like ./ gets converted to .//index.js.

Source:

import type { Context } from '../../context'
import { TEXT_PLAIN } from '../../context'
import type { StreamingApi } from '../../utils/stream'
import { stream } from './'
// ...

Compiled:

import { TEXT_PLAIN } from "../../context.js";
import { stream } from ".//index.js";
// ...
kt3k commented 2 weeks ago

Thanks for the report.

import { TEXT_PLAIN } from "../../context.js";
import { stream } from ".//index.js";
// ...

I confirmed this specifier ".//index.js" exists at /dist/helper/streaming/text.js in hono, and that seems causing the issue.

A more minimal reproduction is:

a.js

import "./b//c.js";

b/c.js

import "../d.js";

d.js

console.log("hi");

When executing a.js, Deno throws the below error:

$ deno a.js 
error: Module not found "file:///path/to/b/d.js".
    at file:///path/to/b//c.js:1:8

(Deno seems considering dir//file is 2 level deeper from dir without normalizing // part)

While Node executes the above without problem.

kt3k commented 2 weeks ago

This looks fixed in Hono's side (https://github.com/honojs/hono/pull/3405 )

@janispritzkau Can you try again with the latest version of Hono?

import "npm:hono@4.6.1/streaming";
marvinhagemeister commented 2 weeks ago

It's great that the hono team simplified the import, but we should be able to deal with ./ imports in npm packages regardless imo.

janispritzkau commented 2 weeks ago

@kt3k Yes, that works. Thank you.

I also think this should be fixed on Deno's end. It seems like extra slashes being ignored in path resolution is a common behavior.

kt3k commented 2 weeks ago

I agree that this should also be fixed on Deno's side. I updated the title to clarify the problem.