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

Incompatibility with subpath imports #1077

Closed
natcohen opened this issue Dec 29, 2021 · 21 comments
Closed

Incompatibility with subpath imports #1077

natcohen opened this issue Dec 29, 2021 · 21 comments

Comments

@natcohen
Copy link

natcohen commented Dec 29, 2021

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.

@lukastaegert
Copy link
Member

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

@guybedford guybedford transferred this issue from rollup/rollup Dec 30, 2021
@guybedford
Copy link
Contributor

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

@shellscape
Copy link
Collaborator

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.

@natcohen
Copy link
Author

natcohen commented Jan 3, 2022

@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.

@ottokruse
Copy link

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.

@shellscape
Copy link
Collaborator

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

@ottokruse
Copy link

ottokruse commented Mar 2, 2022

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
Copy link

ottokruse commented Mar 3, 2022

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"
        }
    }

@natcohen
Copy link
Author

natcohen commented Mar 3, 2022

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

@ottokruse
Copy link

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
Copy link

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)

@stale stale bot added the x⁷ ⋅ stale label May 3, 2022
@stale
Copy link

stale bot commented May 31, 2022

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 stale bot closed this as completed May 31, 2022
@calinalexandru
Copy link

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

@shellscape
Copy link
Collaborator

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.

@calinalexandru
Copy link

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

@shellscape
Copy link
Collaborator

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

@shellscape shellscape reopened this May 31, 2022
@stale stale bot removed the x⁷ ⋅ stale label May 31, 2022
@calinalexandru
Copy link

calinalexandru commented Jun 1, 2022

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?

@stale stale bot added the x⁷ ⋅ stale label Aug 1, 2022
@claytongulick
Copy link

claytongulick commented Aug 4, 2022

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

@stale stale bot removed the x⁷ ⋅ stale label Aug 4, 2022
@stale stale bot added the x⁷ ⋅ stale label Oct 12, 2022
@stale
Copy link

stale bot commented Oct 14, 2022

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 stale bot closed this as completed Oct 14, 2022
@lacherogwu
Copy link

Also works

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

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

@savetheclocktower
Copy link

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.

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

9 participants