denoland / deno

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

[npm] dependencies from `package.json` not installed #19227

Open marvinhagemeister opened 1 year ago

marvinhagemeister commented 1 year ago

It seems like deno doesn't install all packages listed in package.json. There are two packages listed there:

For some reason only mocha is installed, not @babel/register. It's not anywhere in node_modules or node_modules/.deno. This leads to a "module not found" error:

✖ ERROR: Error: Cannot find module '@babel/register'
Require stack:
- /project/node_modules/.deno/mocha@10.2.0/node_modules/mocha/lib/nodejs/esm-utils.js
- /project/node_modules/.deno/mocha@10.2.0/node_modules/mocha/lib/mocha.js
- /project/node_modules/.deno/mocha@10.2.0/node_modules/mocha/lib/cli/one-and-dones.js
- /project/node_modules/.deno/mocha@10.2.0/node_modules/mocha/lib/cli/options.js
- /project/node_modules/.deno/mocha@10.2.0/node_modules/mocha/bin/mocha.js
    at Function.Module._resolveFilename (ext:deno_node/01_require.js:828:15)
    at Function.Module._load (ext:deno_node/01_require.js:675:27)
    at Module.require (ext:deno_node/01_require.js:894:19)
    at require (ext:deno_node/01_require.js:1030:16)
    at exports.requireOrImport (file:///project/node_modules/.deno/mocha@10.2.0/node_modules/mocha/lib/nodejs/esm-utils.js:53:16)
    at async exports.handleRequires (file:///project/node_modules/.deno/mocha@10.2.0/node_modules/mocha/lib/cli/run-helpers.js:94:28)
    at async file:///project/node_modules/.deno/mocha@10.2.0/node_modules/mocha/lib/cli/run.js:349:25 {
  code: "MODULE_NOT_FOUND",
  requireStack: [
    "/project/node_modules/.deno/mocha@10.2.0/"... 42 more characters,
    "/project/node_modules/.deno/mocha@10.2.0/"... 31 more characters,
    "/project/node_modules/.deno/mocha@10.2.0/"... 43 more characters,
    "/project/node_modules/.deno/mocha@10.2.0/"... 37 more characters,
    "/project/node_modules/.deno/mocha@10.2.0/"... 31 more characters
  ]
}

Ran into this when trying to use deno to work on https://github.com/preactjs/preact-render-to-string/ .

Steps to reproduce

  1. Clone https://github.com/marvinhagemeister/deno-babel-register-bug
  2. Run deno run -A npm:mocha -r @babel/register
bartlomieju commented 1 year ago

CC @dsherret I don't remember the exact reason why we didn't install devDependencies by default

dsherret commented 1 year ago

We do

https://github.com/denoland/deno/blob/3d865949c2f9f0cb61031bcc2b9e81a4ca623109/cli/args/package_json.rs#L109-L112

dsherret commented 1 year ago

In this case, running a binary command like...

deno run -A npm:mocha -r @babel/register

...will not cause an "npm install" of the packages. It will only install npm:mocha. As for how to cause an npm install, it is lazily done when some code references a bare specifier that matches what's found in a package.json (so in this case, just running deno run -A npm:mocha -r @babel/register won't work because @babel/register hasn't had a chance to be installed). The reason it's done is because a lot of people use deno for scripts in npm packages.

Few points:

  1. I kind of wonder if we need something like deno cache package.json to force an "npm install".
  2. Perhaps if a require fails for something that's found in the package.json then it could attempt to do an "npm install" at that point. Issue is this might cause some weird edge cases. Probably better to have the ability to do an explicit "npm install"
  3. Maybe we just need to make an install always happen when "nodeModulesDir": true in the deno.json (so fixing this would be to add a deno.json with that in it)
marvinhagemeister commented 1 year ago

I've been pondering about this and the more I think I about the more I lean towards to have an npm install command for npm packages. It would match the expectations folks have around them. URLs should still be downloaded on the fly and for folks who don't want to install npm packages, can use esm.sh.

We tried the auto install of npm packages thing in Preact team when working onwmr around 2020 and parcel tried to do that a little later too. Both projects backtracked on that. In wmr we ran into edge case after edge case and ultimately abandoned the approach around the time we decided to switch to vite anyway.

Problem is that it can get real messy with postinstall scripts which can run anything. There are a couple of packages in the tooling space which rely on it. It's a little dangerous to fire postinstall scripts when the developer accidentally mistyped a popular package name and suddenly it installed a type squatted malicious package and ran their malicious postinstall script.

But the main issue both projects ran into is that people perceived it as very unexpected behavior. That led to folks asking for a way early on how to disable that. See the parcel issue tracker for lots of vocal users.

But this can also be an opportunity for us to integrate the permission system for running those install scripts. I'm wondering if this distinction would make sense:

Note that those users were already node users though and familiar with the install command. I'm not sure how the current deno userbase would react.

dsherret commented 1 year ago

I think it's ok if certain edge cases don't work. The convenience of not needing to worry about an explicit install command outweighs the negatives in my opinion and hopefully package authors who run into edge cases we can't handle will adapt to trying to get their packages to work with Deno. For example, we don't support postinstall scripts atm, but a package author can lazily do some initialization on first run.

Btw, I'm not sure how running an explicit npm install protects against misspelling a package name. Even with npm install --ignore-scripts, you would still have to notice that the package name was mispelled before you execute node main.js.