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

npm: Import map is not supported for "require()" #15948

Closed
bartlomieju opened this issue Sep 18, 2022 · 4 comments
Closed

npm: Import map is not supported for "require()" #15948

bartlomieju opened this issue Sep 18, 2022 · 4 comments

Comments

@bartlomieju
Copy link
Member

We have support for import map in case of ESM imports of npm: specifiers, but it's not supported if a module is required.

This might be a bit exotic, but I believe there are certain valid use cases for it.

Eg. I'm trying to run a Vite project, I did set up following import map:

{
    "imports": {
        "@vitejs/plugin-vue": "npm:@vitejs/plugin-vue@3.0.3",
        "vite-plugin-deno": "./plugin.js",
        "vite": "npm:vite@3.0.7",
        "vue": "npm:vue@3.2.37"
    }
}

This works nicely, as long as there's a package.json file in the same directory like so:

{
  "name": "my-vue-app",
  "private": true,
  "version": "0.0.0",
  "type": "module",
  "scripts": {
    "dev": "vite",
    "build": "vite build",
    "preview": "vite preview"
  },
  "dependencies": {
    "vue": "^3.2.37"
  },
  "devDependencies": {
    "@vitejs/plugin-vue": "^3.0.3",
    "vite": "^3.0.7"
  }
}

However as soon as I remove that package.json file, I'm no longer able to run deno run -A --unstable npm:vite, because I get following error:

failed to load config from /Users/ib/dev/my-vue-app/vite.config.js
error when starting dev server:
Error: Cannot find module 'vite'
Require stack:
- /Users/ib/dev/my-vue-app/vite.config.js
- /Users/ib/Library/Caches/deno/npm/registry.npmjs.org/vite/3.1.2/dist/node/chunks/dep-cff57044.js
    at Function.Module._resolveFilename (deno:ext/node/02_require.js:615:17)
    at Function.Module._load (deno:ext/node/02_require.js:447:29)
    at Module.require (deno:ext/node/02_require.js:658:21)
    at require (deno:ext/node/02_require.js:789:18)
    at Object.<anonymous> (file:///Users/ib/dev/my-vue-app/vite.config.js:31:19)
    at Object.<anonymous> (file:///Users/ib/dev/my-vue-app/vite.config.js:56:4)
    at Module._compile (deno:ext/node/02_require.js:719:36)
    at Object._require.extensions.<computed> [as .js] (file:///Users/ib/Library/Caches/deno/npm/registry.npmjs.org/vite/3.1.2/dist/node/chunks/dep-cff57044.js:63489:24)
    at Module.load (deno:ext/node/02_require.js:636:34)
    at Function.Module._load (deno:ext/node/02_require.js:493:14)

If require() took into account import map file, just like ESM imports do, then we could remove package.json from the project altogether and instead replace it with deno.json + import_map.json.

@bartlomieju
Copy link
Member Author

@guybedford I would appreciate your thoughts on this topic.

@cowboyd
Copy link

cowboyd commented Sep 19, 2022

Potentially related, when loading plugins from a CLI, it would be nice to have more fine-grained control over how imports are mapped when using both ESM and CJS #8655 (comment)

@guybedford
Copy link
Contributor

guybedford commented Sep 22, 2022

@bartlomieju not sure I have much to add on this, but can share my thoughts certainly. In Node.js loader hooks are not currently supported for CommonJS require, so that a userland import map implementation will likely only apply to ESM, although could hook into the require system using the standard monkey patching as well. The new loader work does point in the direction of loaders being able to have more general hooks so that this might be supported in future.

So it may well be possible, but yet to be seen if Node.js implementing import maps would also want to extend that to require. Typically Node.js does aim to support for CommonJS for new resolution features (like we did with "imports" and "exports").

In terms of CommonJS and ESM having different resolutions ("import" and "require" conditions), I've found in my own work that exact module scopes can allow supporting this for the most part, and have yet to hit up against a case of a single CommonJS module that does both import('pkg') and require('pkg') expecting different results. That would be an edge case to consider though.

So overall it seems useful to me, but it could be helpful to ensure it matches what Node.js does itself.

On a related note, has there been any discussion of supporting scoped maps for npm: specifiers? Either by scoping their final file:/// URL or the npm: URL directly?

@bartlomieju
Copy link
Member Author

@guybedford thank you for the time you took to respond. Since opening this issue we managed to overcome problems at hand and I no longer believe we should be adding import map support for require().

n Node.js loader hooks are not currently supported for CommonJS require, so that a userland import map implementation will likely only apply to ESM, although could hook into the require system using the standard monkey patching as well. The new loader work does point in the direction of loaders being able to have more general hooks so that this might be supported in future.

So it may well be possible, but yet to be seen if Node.js implementing import maps would also want to extend that to require. Typically Node.js does aim to support for CommonJS for new resolution features (like we did with "imports" and "exports").

That makes total sense and is inline with browser behavior so I think we should keep it that way.

In terms of CommonJS and ESM having different resolutions ("import" and "require" conditions), I've found in my own work that exact module scopes can allow supporting this for the most part, and have yet to hit up against a case of a single CommonJS module that does both import('pkg') and require('pkg') expecting different results. That would be an edge case to consider though.

Thanks for pointing this out, definitely something to keep an eye on, but again before this edge case is hit I think we should stay put.

On a related note, has there been any discussion of supporting scoped maps for npm: specifiers? Either by scoping their final file:/// URL or the npm: URL directly?

Not yet, but I think this would be very useful for quickly patching dependencies locally. @dsherret any thoughts on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants