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

Conditional exports doesn't respect alternative paths #37928

Open
xbaun opened this issue Mar 26, 2021 · 14 comments
Open

Conditional exports doesn't respect alternative paths #37928

xbaun opened this issue Mar 26, 2021 · 14 comments
Labels
module Issues and PRs related to the module subsystem.

Comments

@xbaun
Copy link

xbaun commented Mar 26, 2021

  • Version: v14.16.0
  • Platform: Darwin DEV135MAC0211.fritz.box 19.6.0 Darwin Kernel Version 19.6.0: Tue Jan 12 22:13:05 PST 2021; root:xnu-6153.141.16~1/RELEASE_X86_64 x86_64
  • Subsystem:

What steps will reproduce the bug?

When importing a package with conditional exports declared in the package.json, then node doesn't respect alternative path exports:

"exports": {
    "./*": {
        "require": [
            "./dist/cjs/*/index.js",
            "./dist/cjs/*.js"
        ]
    }
}

Node only tries to resolve the first one and fails immediately if the first path to a module doesn't exist. I don't know if this is the intended behaviour, but in the Webpack guide it is explicitly mentioned as a feature (https://webpack.js.org/guides/package-exports/#alternatives).

What is the expected behavior?

Node tries to resolve an import against each entry in the conditional exports array and returns the first successful loaded module.

What do you see instead?

Node only resolves the first entry of the conditional exports alternatives.

@Ayase-252 Ayase-252 added the module Issues and PRs related to the module subsystem. label Mar 26, 2021
@xbaun
Copy link
Author

xbaun commented Mar 26, 2021

I debugged into resolve.js and it seems that it is only checked if the resolved path result of the current target is not undefined or null, but not if it actually exists.

@aduh95
Copy link
Contributor

aduh95 commented Mar 27, 2021

@nodejs/modules

@jkrems
Copy link
Contributor

jkrems commented Mar 28, 2021

Exports is generally designed to resolve unambiguously without hitting the disk. So what you're observing is working as intended: As soon as we find a valid file name, we assume that it's the intended target. It explicitly doesn't matter if the file currently exists or not. E.g. it would be valid to resolve to a non-existing path, create the file, and then load it.

@xbaun
Copy link
Author

xbaun commented Mar 28, 2021

@jkrems Tanks for the explanation. That would mean that the this feature only applies to Webpack when resolving module paths.

Then directly a follow-up question: When the cjs loader is loading a CommonJS module with disabled package encapsulation by using the exports field in package.json, then the cjs loader doesn’t try to resolve a directory import to an index.js file (see example below). So in the end the cjs loader is acting like the esm loader as described in the cjs loader pseudo algorithm.

I would have assumed now, as this is a CommonJS module, that the normal directory resolution behavior of the cjs loader is applied. Since this is not explicitly mentioned in the node docs (at least I didn't find it), does this mean that using the exports field in the package.json will disable the resolution of index.js directory files in CommonJS Modules? Moreover, I wonder about this because the node docs also says that "exports" are not specific to ES modules or CommonJS

Additionally, there is also the --experimental-specifier-resolution=node flag in v15 which enables the CommonJS loading behaviour in ESModules, but as it seems, it is not working in conjunction with the exports field when package encapsulation is disabled... (this is more of a remark than a question 😬 )


Package.json

"name": "foo",
"exports": {
    "./*": {
        "require": [
            "./dist/cjs/*.js"
        ]
    }
}

then

require('foo/bar') // should load foo/dist/cjs/bar/index.js if bar is a directory

@aduh95
Copy link
Contributor

aduh95 commented Mar 28, 2021

"exports" field make foo/bar resolve to ./node_modules/foo/dist/cjs/bar.js. The fact that there's a bar directory is irrelevant here. If there is a bar.js directory, you'll get a Cannot find module error message, even if the bar.js directory contains an index.js file, or a package.json.

You can add a "./bar" entry to you export map to customize its resolution:

{
  "name": "foo",
  "exports": {
    "./bar": "./dist/cjs/bar/index.js",
    "./*": {
        "require": [
            "./dist/cjs/*.js"
        ]
    }
  }
}

@xbaun
Copy link
Author

xbaun commented Mar 29, 2021

@aduh95 That’s what I meant. In the end, I want to export all files from a transpiled directory including the directory paths with the index.js. Packages using this one, can then pick the modules they need. It would be really tedious for large packages to enter all directory exports manually into the exports field (apart from the fact that it would not be well maintainable in the long run) or to write a script here that would enter it automatically would be a bit overengineered.

It took me some time to realize that Node handles CommonJS modules differently than before when using exports, and that they then behave like ES modules. This change in behaviour is also not directly apparent from reading the Node documentation.

Thanks for the clarification.

@aduh95
Copy link
Contributor

aduh95 commented Mar 29, 2021

It would be really tedious for large packages to enter all directory exports manually into the exports field (apart from the fact that it would not be well maintainable in the long run) or to write a script here that would enter it automatically would be a bit overengineered.

Yes the "exports" field was designed for package encapsulation first, for package authors who want to be picky regarding what is exposed to their users. If you are looking to get the old behaviour back, it requires a bit of hacking I reckon. You could also have a /dist/cjs/bar.js file that contains module.exports=require("./bar/") if that helps.

It took me some time to realize that Node handles CommonJS modules differently than before when using exports, and that they then behave like ES modules. This change in behaviour is also not directly apparent from reading the Node documentation.

Maybe the documentation could use a bit of improvement to remove the ambiguity here. If you are willing to open a PR for that, that'd be awesome :)

@xbaun
Copy link
Author

xbaun commented Mar 29, 2021

If you are looking to get the old behaviour back, it requires a bit of hacking I reckon. You could also have a /dist/cjs/bar.js file that contains module.exports=require("./bar/") if that helps.

I didn't think of that - simple workaround that can be used by cjs and esm alike 👍

Maybe the documentation could use a bit of improvement to remove the ambiguity here. If you are willing to open a PR for that, that'd be awesome :)

I'll take a look at it and add a few lines when I have a little more time.

@zheeeng
Copy link

zheeeng commented Sep 8, 2021

Have the same expection, the most common usage is leverage the exports field to import the dist artifacts without the dist folder name disappearing in path.

import foo from 'foo/baz' resolves to import foo from 'foo/baz/index.js'
import foo from 'foo/bar/baz' resolves to import foo from 'foo/bar/baz.js'

@jasonkuhrt
Copy link

jasonkuhrt commented Nov 19, 2021

Can someone share a link where I can read about the rationale for the design of "target" alternatives? Based on this thread it seems that for a vanilla package consumed by Node they are not useful. Curious to learn more. I am in a tool author context here so looking for deeper details beyond day to day usage from consumer point of view.

@privatenumber
Copy link
Contributor

@jkrems

Your proposal spec explains how the fallback array can be used:

{
  "exports": {
    "./core-polyfill": ["std:core-module", "./core-polyfill.js"]
  }
}

Whenever there is a validation failure, any exports match must throw a Module Not Found error, and any validation failure context can be included in the error message.

Fallback arrays allow validation failures to continue checking the next item in the fallback array providing forwards compatiblitiy for new features in future based on extending these validation rules to new cases.

Wouldn't "validation failures" include the "Module Not Found error" mentioned in the sentence above?

@alshdavid
Copy link

alshdavid commented Sep 29, 2023

As we transition to a module world from a commonjs world - I would like to offer consumers of my npm packages both CJS and MJS entry points, loading transparently regardless of their preference - maintaining a consistent import experience that preserves the natural import behaviour expected by consumers.

This is essentially "differential loading" in Node (CJS/ESM) and I would like this behaviour to be retained when I eventually stop distributing CJS code entirely. To do this I (want to) use conditional exports and subpattern exports.

From the consumer's perspective:

If importing a package from a CJS context, they would expect the following resolution rules:

const pkg = require('pkg')             /** resolves to */ 'node_modules/pkg/require/index.js'

const pkg = require('pkg/index')       /** resolves to */ 'node_modules/pkg/require/index'   
                                                /** or */ 'node_modules/pkg/require/index.js'
                                                /** or */ 'node_modules/pkg/require/index/index.js'

const pkg = require('pkg/index.js')    /** resolves to */ 'node_modules/pkg/require/index.js'

const pkg = require('pkg/foo.js')      /** resolves to */ 'node_modules/pkg/require/foo.js'

const pkg = require('pkg/foo')         /** resolves to */ 'node_modules/pkg/require/foo'    
                                                /** or */ 'node_modules/pkg/require/foo.js'
                                                /** or */ 'node_modules/pkg/require/foo/index.js'

const pkg = require('pkg/foo/bar')     /** resolves to */ 'node_modules/pkg/require/foo/bar' 
                                                /** or */ 'node_modules/pkg/require/foo/bar.js'
                                                /** or */ 'node_modules/pkg/require/foo/bar/index.js'

const pkg = require('pkg/foo/bar.js')  /** resolves to */ 'node_modules/pkg/require/foo/bar.js'

And if importing the package from a MJS context, they would expect the following resolution rules:

import pkg from 'pkg'               /** resolves to */ 'node_modules/pkg/import/index.js'
import pkg from 'pkg/index'         /** not valid, requires exact match (file extension in this case) */
import pkg from 'pkg/index.js'      /** resolves to */ 'node_modules/pkg/import/index.js'
import pkg from 'pkg/foo.js'        /** resolves to */ 'node_modules/pkg/import/foo.js'
import pkg from 'pkg/foo'           /** not valid, requires exact match (file extension in this case) */
import pkg from 'pkg/foo/bar.js'    /** resolves to */ 'node_modules/pkg/import/foo/bar.js'

In concept, a package.json that looks like this should suffice:

{
  "name": "pkg",
  "exports": {
    ".": {
      "import": "./import/index.js",
      "require": "./require/index.js"
    },
    "./*": {
      "import": "./import/*",
      "require": ["./require/*", "./require/*.js", "./require/*/index.js"]
    }
  }
}

However due to Node returning the first item in the ["./require/*.js", "./require/*/index.js"] array, I am unable to recreate the natural CJS resolution algorithm.

While I understand that Node is try to minimize hitting the FS as much as possible - I believe supporting this use case is vital in transitioning from commonjs to modules as library maintainers can better distribute packages with both formats such that they behave consistently with expectations.

Looking at the resolution and loading algorithm described here: https://nodejs.org/api/esm.html#resolution-and-loading-algorithm

If PACKAGE_RESOLVE added a condition for accepting an Array<string>, testing each pattern until success where PACKAGE_EXPORTS_RESOLVE could return a Array<string> - this would only affect imports like the ones I describe here.

This would not perform any worse than CJS imports that exclude extensions or import folders do currently - because Node already needs to test the FS for those alternative paths.

@privatenumber
Copy link
Contributor

I believe you should just be able to do:

{
    "./*": {
      "import": "./import/*",
      "require": "./require/*"
    }
}

Because it's the CommonJS resolution algorithm that attempts the extension-less and directory index paths.

@alshdavid
Copy link

alshdavid commented Oct 2, 2023

Because it's the CommonJS resolution algorithm that attempts the extension-less and directory index paths.

I would have expected so too however it does not work that way in practice:

Example repo (no-array branch)

Where I set the require property as "require": "./require/*" (the same as described in your comment)

/project/node_modules/pkg/package.json

/project/src/cjs/case-1.js <- does not work

/project/src/cjs/case-1_1.js <- works

Note that I have an additional superficial package.json in the /project/node_modules/pkg/require/package.json path to tell Node to consume it as commonjs. This is the recommended method as I understand it.

I would expect:

require('pkg/foo')  /** resolves to */ 'node_modules/pkg/require/foo'   
                             /** or */ 'node_modules/pkg/require/foo.js' // <---- Match this
                             /** or */ 'node_modules/pkg/require/foo/index.js'

However Node does not fall back to alternative imports when coming from a commonjs consumer, instead I get the error:

$ node ./src/cjs/case-1.js 
node:internal/modules/cjs/loader:573
      throw e;
      ^

Error: Cannot find module '/Users/alshdavid/Development/alshdavid-scratch/node-exports-subpath-pattern-bug-example/node_modules/pkg/require/foo'
    at createEsmNotFoundErr (node:internal/modules/cjs/loader:1098:15)
    at finalizeEsmResolution (node:internal/modules/cjs/loader:1091:15)
    at resolveExports (node:internal/modules/cjs/loader:567:14)
    at Module._findPath (node:internal/modules/cjs/loader:636:31)
    at Module._resolveFilename (node:internal/modules/cjs/loader:1063:27)
    at Module._load (node:internal/modules/cjs/loader:922:27)
    at Module.require (node:internal/modules/cjs/loader:1143:19)
    at require (node:internal/modules/cjs/helpers:110:18)
    at Object.<anonymous> (/Users/alshdavid/Development/alshdavid-scratch/node-exports-subpath-pattern-bug-example/src/cjs/case-1.js:2:17)
    at Module._compile (node:internal/modules/cjs/loader:1256:14) {
  code: 'MODULE_NOT_FOUND',
  path: '/Users/alshdavid/Development/alshdavid-scratch/node-exports-subpath-pattern-bug-example/node_modules/pkg/package.json'
}

Node.js v18.17.0

Indicating that it only tested pkg/require/foo

If I change case-1.js to require the same path using a relative path specifier, it works as expected:

const { foo } = require('../../node_modules/pkg/require/foo')
console.log(foo)

Have I misconfigured my package? Is this a bug or expected behaviour?

If it's a bug and we expect the commonjs resolution algorithm to work here then my comment about using arrays to recreate it is not relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

8 participants