plugins icon indicating copy to clipboard operation
plugins copied to clipboard

Incompatibility with subpath imports

Open natcohen opened this issue 3 years ago • 18 comments

Rollup Version

2.62.0

Operating System (or Browser)

Chrome

Node Version (if applicable)

15.4.0

Link To Reproduction

Check here

Expected Behaviour

Rollup should support by default the subpath imports feature provided NodeJS.

Actual Behaviour

In advance, sorry but this bug requires the package.json and can't be reproduced without it.

My library takes advantage of the NodeJS subpath imports. My package.json looks like this:

{
  "name": "mylib",
  "version": "0.0.1",
  ...
  "imports": {
    "#db/*": "./db/*.js"
  }
  ...
}

and in my code, I import internal files as follows:

import db from "#db/connect";

Unfortunately, when I try to bundle my code, I get an Unresolved dependencies error from Rollup. It can't resolve the #db/connect path.

For reference, here is my rollup.config.js:

import resolve from "rollup-plugin-node-resolve";
import commonjs from "rollup-plugin-commonjs";
import babel from "rollup-plugin-babel";
import pkg from "./package.json";

export default [
    {
        input: "src/index.js", // entry point
        output: {
            name: "mylib", // package name
            file: pkg.browser,
            format: "umd",
        },
        plugins: [
            resolve(),
            commonjs(),
            babel({
                exclude: ["node_modules/**"],
            }),
        ],
    },
    {
        input: "src/index.js", // entry point
        output: [
            { file: pkg.main, format: "cjs" },
            { file: pkg.module, format: "es" },
        ],
        plugins: [
            babel({
                exclude: ["node_modules/**"],
            }),
        ],
    },
];

Rollup should support the subpath import feature provided NodeJS by default.

natcohen avatar Dec 29 '21 09:12 natcohen

This would be a feature request for @rollup/plugin-node-resolve

lukastaegert avatar Dec 30 '21 20:12 lukastaegert

I'm happy to review any PR work here, it would be great to see this supported.

guybedford avatar Dec 30 '21 20:12 guybedford

Thanks for opening an issue. Citing the issue template:

🚨 Issues WITHOUT a valid reproduction WILL BE CLOSED!

Please provide one by:

  1. Using the REPL.it plugin reproduction template at https://repl.it/@rollup/rollup-plugin-repro
  2. Provide a minimal repository link (Read https://git.io/fNzHA for instructions). Please use NPM for installing dependencies! These may take more time to triage than the other options.
  3. Using the Rollup REPL at https://rollupjs.org/repl/

⚠️ ZIP Files are unsafe and maintainers will NOT download them.

We cannot make this any clearer. Please add a reproduction and we'll be happy to triage further.

Since this issue was transferred to this repo, please note the message above - we require a reproduction without exception. There are options above which provide the means to create a reproduction with package.json.

shellscape avatar Dec 30 '21 20:12 shellscape

@shellscape Thanks for the links. I was able to create a reproduction here.

@guybedford Unfortunately, I'm just trying for the first time rollup and don't have the knowledge to propose a PR.

natcohen avatar Jan 03 '22 14:01 natcohen

Subpath imports are a great feature to write code that works for both Node.js and Web (by having a nice mechanism to separate out Node.js/Web specific code). Both esbuild and webpack support this btw.

ottokruse avatar Mar 02 '22 19:03 ottokruse

@ottokruse we're very open to contributions for enhancing @rollup/plugin-node-resolve

shellscape avatar Mar 02 '22 19:03 shellscape

Workaround while we're waiting for a hero to create a PR to @rollup/plugin-node-resolve is to use @rollup/plugin-alias:

import alias from "@rollup/plugin-alias";
export default {
  // ...
  plugins: [
    alias({
      entries: [
        {
          find: "#my-subpath-import-name",
          replacement: "./relative-path-to-the-js-file-to-actually-use",
        },
      ],
    }),
  ],
}

It be nicer if you didn't have to write this config but at least there's an escape hatch.

ottokruse avatar Mar 02 '22 20:03 ottokruse

Update: having looked at this again, a bit closer this time, subpath imports are in fact supported already 🎉 by @rollup/plugin-node-resolve.

I've configured rollup as follows now, and this nicely bundles my library, that uses subpath imports:

import resolve from "@rollup/plugin-node-resolve";
export default {
  // ...
  plugins: [resolve({ browser: true })],
}

I believe this issue can be closed.

Note, for those interested, this is the subpath import in my lib's package.json that needed the above:

    "imports": {
        "#my-subpath-import-name": {
            "browser": "./browser-impl.js",
            "default": "./node-impl.js"
        }
    }

ottokruse avatar Mar 03 '22 20:03 ottokruse

hum... are we talking about the same thing here? before we celebrate, could you please reproduce it? There is already a reproduction example here

natcohen avatar Mar 03 '22 20:03 natcohen

Think we are talking about the same thing yes. Quick look at the repro sample, it uses https://www.npmjs.com/package/rollup-plugin-node-resolve and not @rollup/plugin-node-resolve. Might be it? Otherwise it also uses subpath imports, albeit a slightly different use (with * syntax)

ottokruse avatar Mar 03 '22 20:03 ottokruse

Yeah it seems to really work now 😄 https://replit.com/@OttoKruse/rollup-plugin-repro (same case, slightly tweaked, updated rollup and used right node resolve lib)

ottokruse avatar Mar 03 '22 20:03 ottokruse

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it.

stale[bot] avatar May 31 '22 07:05 stale[bot]

Hello all,

Please correct me if I'm mistaking, but isn't @rollup/plugin-node-resolve main purpose to locate and resolve node_modules? From the REPL provided by @ottokruse I don't see any dependencies, so I don't really understand what are we testing. I've made another test https://replit.com/@terrorb1ade/subpath-imports-rollup-test?v=1, and it seems to me that indeed https://nodejs.org/api/packages.html#subpath-imports are not considered correctly. @shellscape

Also git repo: https://github.com/calinalexandru/subpath-imports-rollup-test

calinalexandru avatar May 31 '22 12:05 calinalexandru

please read the bot's message carefully. this wasn't closed because it was invalid, this was closed because for 2 months no maintainers had time to work on it. That's a signal that our current maintainers likely won't have time in the foreseeable future. if you would like to see traction on this, please recruit some folks who have the time to work on it. many folks don't agree with use of stale bot, and that's okay, however we do use it in this repository.

shellscape avatar May 31 '22 12:05 shellscape

I left this thread open since yesterday while I did some research on the issue; haven't really paid attention to the stale bot, I will make sure to do in the future. I think rollup is amazing along with all it's plugins, so I'll dig deeper into the code and hopefully provide a decent PR. Thank you @shellscape

calinalexandru avatar May 31 '22 12:05 calinalexandru

okay cool, thanks for the kind words as well. I'll reopen this in anticipation and we'll consider it active again

shellscape avatar May 31 '22 12:05 shellscape

Good news! All the logic regarding Subpath Imports is already present & documented. From the docs using exportConditions, it states that

In order to get the resolution behavior of Node.js, set this to ['node'].

I've tested that using nodeResolve({ exportConditions: ['node'] }) will instruct the plugin to use node subimports instead of default which is usually the browser version. Works perfectly and no changes are required.

The problem that arises, I think, besides that people might not read the docs carefully, there is a configuration conflict with browser option which is false by default. Seeing that browser resolution is false you think that node resolution will be set by default - but it's not, since you have to set it explicitly at stated in the docs.

I'm not sure about the correct steps to be taken, my first thought is bump the plugin to a new version and change default options, what you guys think?

calinalexandru avatar Jun 01 '22 19:06 calinalexandru

Disregard - this seems like a different issue - also, why can't I delete a comment? 😄

claytongulick avatar Aug 04 '22 17:08 claytongulick

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it.

stale[bot] avatar Oct 14 '22 04:10 stale[bot]

Also works

import { nodeResolve } from '@rollup/plugin-node-resolve';

export default {
	plugins: [
		nodeResolve({
			resolveOnly: module => module.startsWith('#'),
		}),
	],
};

lacherogwu avatar Dec 01 '22 20:12 lacherogwu

I dove into this and it seems to be a problem with only some subpath imports.

If you define them as follows:

{
  "imports": {
    "#internal/*": "./src/*.js"
  }
}

…then the node-resolve plugin can understand them. But if you put the extension at the end…

{
  "imports": {
    "#internal/*.js": "./src/*.js"
  }
}

…the plugin doesn't know what to do.

Consider the resolvePackageImportsExports function. I think it envisions three kinds of import keys: those without wildcards, those that end with wildcards, and those that end with path separators.

But the node docs specifically envision, and even prescribe, keys with wildcards that are followed by the file extension. These are falling through the cracks in resolvePackageImportsExports.

In my case, when I rewrote my internal imports to remove file extension, and rewrote my package.json to look like the first example, everything started working.

@shellscape, I think this needs to be re-opened. The plugin does not understand wildcard subpath imports when the wildcard does not start or end the identifier.

savetheclocktower avatar Jan 30 '23 23:01 savetheclocktower