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

Add mainDir to module resolution algorithm #14970

Closed
davewasmer opened this issue Aug 22, 2017 · 52 comments
Closed

Add mainDir to module resolution algorithm #14970

davewasmer opened this issue Aug 22, 2017 · 52 comments
Labels
feature request Issues that request new features to be added to Node.js. module Issues and PRs related to the module subsystem.

Comments

@davewasmer
Copy link

First off, I realize that the Module API is frozen, so I understand if this proposal will be rejected outright. However, I figured I'd give it a shot anyway.

Many node packages today are written in a source language other than JavaScript, and compiled before publishing. The main field in the package.json file allows these compiled packages to specify a different file as the entry point (i.e. dist/index.js).

However, the module resolution algorithm allows for loading files within a module via path syntax, i.e. require('foo/bar/quux') would load bar/quux.js from the foo module. Currently, the only way to ensure this kind of path syntax works (without needing to include dist/ in the path) is to publish only the dist folder. This approach requires the author to remember to publish the subdirectory every time, which can be mistake prone.

I'd like to propose supporting a new field in the package.json spec called mainDir. If present, the module resolution algorithm would treat that mainDir path as the root path for that package, so compiled packages could specify "mainDir": "dist", and easily support sub-path module loading (i.e. require('foo/bar/quux')) without having to remember to publish from the subdirectory every time.

I think this would be a relatively straightforward change on the implementation side, happy to PR it, but I wanted to test the waters first. The only trouble I can see is if people are already using mainDir in package.json files for something else. However, a quick Github search reveals zero public instances of using mainDir in a package.json file. This of course doesn't preclude it's use in private repos, but I think it's a strong indicator that we wouldn't be trampling on too much, if any, existing code.

@davewasmer
Copy link
Author

Some prior art: #3953. That issue was closed, but the reasons given were focused on the fact that it wanted to change the existing semantics of the main field, which would be problematic for backwards-compat. My proposal differs in suggesting we claim a new, essentially unused field for purely opt-in semantics.

@joyeecheung joyeecheung added the module Issues and PRs related to the module subsystem. label Aug 22, 2017
@mscdex mscdex added the feature request Issues that request new features to be added to Node.js. label Aug 22, 2017
@davewasmer
Copy link
Author

I created a package that simulates the semantics I'm proposing via monkey-patching Module, for the curious: https://github.com/davewasmer/main-dir

@Slayer95
Copy link

It seems that you could also be served well by some sort of opt in towards using main as the resolution root. This is what require.main.require() achieves. Have you considered using that?

You could prepend to your files require = require.main.require, and proceed as usual.

@davewasmer
Copy link
Author

@Slayer95 - I wasn't aware of require.main.require(), thanks for pointing that out! However, I'm not sure it will satisfy the use cases I've outlined, if I'm understanding it correctly (please correct me where I go wrong).

Let's say I have a Node script I run via $ node my-app/index.js. My understanding is that now require.main is set to the Module instance for my-app/index.js. If, from inside my-app/index.js, I attempt to deep-load a node_module (i.e. require('foo/bar/quux')), using require.main.require() won't work. That's because it will continue to load relative to my-app/index.js, when I really want it to load relative to my-app/node_modules/foo/dist.

@Slayer95
Copy link

Oh, it seems I missed some bits in your explanation of the problem.

Indeed, the feature I mention would only be useful at the application / child process level. Without intention to hijack the thread, as I'd prefer mainDir, an alternative feature proposal could be a require.packageMain module reference.

@championswimmer
Copy link

Would definitely love to see this happen!
Very useful for transpiled source packages

@igoodrich
Copy link

This may help. For a typescript package I'm working on, I executed the following workaround and it seems to work but is messier than I would like. tbh, I was prety disappointed (and frankly shocked) that this wasn't well supported already.

  • My typescript is in src
  • in tsconfig I include src/**/* and have an outdir of "."
  • this means the root of my project has the transpiled output of everything in src.
  • messy? yes - but the imports are sensible and everything is directories, so not horrible horrible
  • Last thing I did was in package.json I used the "files" bit to include only the files I wanted of the transpiled src -- this means the package is actually kind of clean (and again has the import behavior I'm looking for from the client side -- i.e., without an intermediating "dist" in front of everything).

The net net is that I can npm install ../my-package and that works AND I can npm install git://blah and that works. I have not tried publishing to npm but I imagine if the git works, that'll work.

tsconfig
{ "compilerOptions": { "preserveConstEnums": true, "strictNullChecks": true, "sourceMap": true, "target": "es5", "outDir": ".", "module": "CommonJS", "moduleResolution": "node", "lib": ["es2015"], "rootDir": "./src", "declaration": true }, "include": [ "src/**/*" ], "exclude": [ "node_modules/**/*" ] }
and package.json
{ "name": "t40b-lib", "version": "0.0.1", "description": "", "main": "index.js", "types": "index.d.ts", "scripts": { "build": "tsc", "test": "echo \"Error: no test specified\" && exit 1" }, "files": [ "package.json", "index.js", "index.d.ts", "models", "shared" ]

@GongT
Copy link

GongT commented Apr 17, 2018

I'm using typescript, compile all file from ./src to ./dist

in package.json, run a script when prePublish:

  1. copy package.json & src to dist.
  2. replace all "../src" to "./src" in all .map file (with find and sed)
  3. run npm publish in dist
  4. exit 1 (prevent publish parent folder)

BOOM.

@monitz87
Copy link

This would indeed VERY nice to have

@lazeebee
Copy link

Wouldn't this make conflicts with node_modules paths?

@devsnek
Copy link
Member

devsnek commented Jan 30, 2019

since you can now create arbitrary require functions (https://nodejs.org/api/modules.html#modules_module_createrequirefrompath_filename) can this be considered fixed?

@nixxquality
Copy link

The problem isn't you using require, it's having other people use require for your module.
Making people add a new require stub to use your package is not a solution.

@nfroidure
Copy link

nfroidure commented Feb 28, 2019

This is a really common issue. Each time you need to access some functions that ain't directly exposed by the main field, you end with with pretty ugly imports from the dist folder (see https://github.com/nfroidure/whook/blob/master/packages/whook-cors/src/index.js#L4 for a real use case).

The current solutions then are:

The problem I can see for the mainDir proposal is that then one could not be able to require package.json. It would create the need to handle extra cases in the module resolver for those "not in mainDir anymore" files.

Also, there is the use of the browser field by several bundlers for frontend builds and the browser builds are not necessarily in the same folder.

So, I think the more straightforward solution to this problem would be to introduce a resolveMode field where one could put relative and defaulting to normal. So that requiring a module in a sub path would:

  • resolve to the main file (say dist/index.js, browser bundler would do the same using the browser field instead)
  • apply that path relatively to the main file dirname

The fact that it concerns only sub paths is important so that one requiring modulename/package would still be able to require the package. Or maybe, allow requiring modulename/../package?

WDYT?

@trusktr
Copy link
Contributor

trusktr commented Apr 3, 2019

Yeah, it'd be nice to have this feature so that we can write import Foo from 'my-lib/core/Foo' instead of import Foo from 'my-lib/dist/core/Foo'! Or with CommonJS, write const Foo = require('my-lib/core/Foo') instead of const Foo = require('my-lib/dist/core/Foo').


What I've been doing to publish built files so that it works like we want, is just outputting the build output into the root of my package (mixing them with the root files). It sounds messy, but the following is what I'm doing to manage it (here's the project for reference, infamous (EDIT: this is not longer true for that project, and now I publish from dist similar to what @GongT suggested above), run npm i && npm run build to see what it does).

First, I set up .gitignore to ignore everything in the project (this ignores build output files in the root of the project) and then I whitelist the top-level things I don't want to ignore:

### ignore everything
/*

# including hidden files
/**/.*

### except for the following top-level whitelisted items:

# folders
!/src
!/tests
!/docs
!/examples
!/website

# global build, to be published
!/global.js
!/global.js.map

# config files
!/.all-contributorsrc
!/.builderrc
!/.editorconfig
!/.gitignore
!/.npmignore
!/.npmrc
!/bower.json
!/builder.config.js
!/package.json

# the basics!
!/README.md
!/LICENSE

Then, I simply build everything into the root of my project (gotta be careful not to name any files inside my src folder with the same name as anything in the root of the project).

So, with that gitignore configuration applied, when I build the project the file structure looks like the following, with the grayed-out items being the ignored build output:

Screen Shot 2019-04-03 at 10 08 19 AM

With this setup we can write import Sphere from 'infamous/core/Sphere' instead of import Sphere from 'infamous/dist/core/Sphere'. Or with CommonJS, we can write const Sphere = require('infamous/core/Sphere') instead of const Sphere = require('infamous/dist/core/Sphere').

@devsnek
Copy link
Member

devsnek commented Apr 3, 2019

why not just publish the dist folder instead of publishing the root folder?

@wKich
Copy link
Contributor

wKich commented Apr 3, 2019

@devsnek, because the package.json is located in root directory.

@devsnek
Copy link
Member

devsnek commented Apr 3, 2019

so why not just add a cp to your build script?

@GongT
Copy link

GongT commented Apr 4, 2019

@devsnek I's ugly.

@wKich
Copy link
Contributor

wKich commented Apr 4, 2019

@devsnek, and you cannot simply run npm publish from root dir. You should define deploy script, where you copy all needed files (README.md, LICENSE, etc), and repeat that for pack/link commands.

@andykais
Copy link

andykais commented Apr 4, 2019

@devsnek if you want to be cross platform, this introduces new dependencies. E.g. copy or copy-webpack-plugin

@devsnek
Copy link
Member

devsnek commented Apr 4, 2019

that doesn't seem like the end of the world. i'm just trying to think of solutions that move the performance overhead to build time instead of runtime.

@nfroidure
Copy link

Maybe that the simpler would be to allow the files field to be a map instead of an array where, the sources in the project and their destinations in the package would be set ? That way, no need to cp anything.

Something like:

{
  files: {
    'dist/**': '.'
  }
}

@targos
Copy link
Member

targos commented Apr 4, 2019

@nfroidure Maybe, but that would be a feature request for npm.

@guybedford
Copy link
Contributor

Agreed this sounds like a publish-time feature to me not an install-time one.

Perhaps make a feature request to npm or yarn for a publishDir in the package.json or similar.

@chyzwar
Copy link

chyzwar commented Apr 14, 2019

@guybedford This is not publish-time feature. If this is done only during publish then common workflows like npm link and monorepo scenarios would be broken. See below issues for reference:

lerna/lerna#1282

Above feature request is intended to solve three common problems:

  1. Runtime overhead of accidental require of unused modules. It is common in node js application to require hundreds of unused modules. For browser it is common to have problems with tree-shaking. For modules that expose multiple functions index.js as an entry point in do not work very well.
    Tree-shaking for production build is not working properly angular/components#4137
    tree-shaking angular/angular-cli#6676

  2. Complicated code organization. Current node resolution force library authors to expose everything through named exports, namespaced object or having everything in root. One of the main reason of monorepo popularity is poor node module system.
    https://github.com/lodash/lodash

  3. Leaky module API. It is common that library authors would allow importing modules using specific path. At this point, common /lib/ or /dist/ are leaky abstraction by exposing internal representation of the module. Any change to folder structure become breaking change.
    https://github.com/chriso/validator.js

I will update to provide more examples for above.

@devsnek
Copy link
Member

devsnek commented Apr 14, 2019

seems like subdirectory publishing works well enough for sindre https://github.com/sindresorhus/np

@borekb
Copy link

borekb commented Aug 29, 2019

Maybe the new exports field in package.json resolves this? I just learnt about it from #21787 (comment), seems exactly like what we'd use.

@chyzwar
Copy link

chyzwar commented Aug 29, 2019

using exports field in package.json can we map lib folder like this?

  "exports": {
    "./": "./lib/"
  }

@zakkudo
Copy link

zakkudo commented Aug 29, 2019

As a reminder to those on the thread: If your solution is talking about an index.js your answer most likely has nothing to do with with is being requested.

Yes, it is possible to copy files around and get a package published in a desired format. But because this hack is not built into npm, you cannot do a direct git install anymore because the paths will not match up. If you're copying your package.json around, using the increment version functionality does not work correctly. On the bright side, exports does look like the closest thing to a solution.

@andykais
Copy link

andykais commented Sep 3, 2019

@chyzwar that does in fact work! Here is a slightly expanded answer.

nice-exports-sample project structure:

package.json
src/
  index.js
  another.js
  hidden/module.js
build/
  index.js
  another.js
  hidden/module.js

package.json

{
  "type": "module",
  "main": "./build/index.js",
  "exports": {
    "./": "./build/"
  }
}

then, in a project which uses this module:

// main alias:
import { main } from 'nice-exports-sample'
// exports aliases:
import { main as mainAgain } from 'nice-exports-sample/index.js'
import { another } from 'nice-exports-sample/another.js'
// nested folders do not resolve, this line would fail
// import { hidden } from 'nice-exports/hidden/module.js'

I ran this code with node 12 and --experimental-modules, though it likely works without using experimental modules.

[edit] I know this isn't everyones intended use case (many users want to avoid leaking internal modules). This example is really just a way to avoid copying package.json into your build folder and publishing from there.

@suchipi
Copy link

suchipi commented Sep 25, 2019

Publish-time workaround doesn't work in a monorepo with symlinks :\

@trusktr
Copy link
Contributor

trusktr commented Jan 1, 2020

EDIT: this comment is wrong. The package.json exports field now provides the solution, which was not the case at the time @andykais wrote his last comment. Skip to the end.


The new exports option almost gets us there. What if we want to publish everything, including sub folders? If we write

{
  "type": "module",
  "main": "./dist/index.js",
  "exports": {
    "./": "./dist/"
  }
}

then we can not import {something} from 'package/sub/folder/foo.js.

To make it work, we'd need to write

{
  "type": "module",
  "main": "./dist/index.js",
  "exports": {
    "./": "./dist/",
    "./foo": "./dist/foo",
    "./foo/bar": "./dist/foo/bar",
    "./foo/bar/baz": "./dist/foo/bar/baz",
    "./lorem": "./dist/lorem",
    // ...
  }
}

It is ironic that this new exports feature came out, with only a partial solution.

@trusktr
Copy link
Contributor

trusktr commented Jan 1, 2020

deleted

@trusktr
Copy link
Contributor

trusktr commented Jan 1, 2020

deleted

@trusktr
Copy link
Contributor

trusktr commented Jan 6, 2020

@BridgeAR There's a large number of up votes for this in the comments after you closed this. Can you please re-open this?

@MylesBorins
Copy link
Contributor

@trusktr I'm a bit confused by #14970 (comment)

Once you map a folder you can deeply traverse it, you shouldn't have to map each subsequent folder... afaict the requested feature here is 100% implementable /w export maps. Photos attached of an example

Screen Shot 2020-01-06 at 12 00 15 AM

Screen Shot 2020-01-06 at 12 00 28 AM

@BridgeAR BridgeAR reopened this Jan 6, 2020
@BridgeAR
Copy link
Member

BridgeAR commented Jan 6, 2020

Reopened for further discussion.

@trusktr
Copy link
Contributor

trusktr commented Jan 8, 2020

@MylesBorins Your example is using CommonJS modules. I made a small example of how it fails with ES Modules here: https://github.com/trusktr/es-modules-exports-alias-unable-to-import-subfolder

@MylesBorins
Copy link
Contributor

@trusktr I just tested on my local machine and both examples work fine. Are you using Node.js 13?

There is no difference in how export maps works between ESM and CJS and they are built to model how import maps will work. As mentioned above the behavior you are asking for already exists... and afaict your demo works fine.

Screen Shot 2020-01-07 at 8 24 50 PM

@MylesBorins
Copy link
Contributor

barring any new evidence I think that this issue should be clsoed

@trusktr
Copy link
Contributor

trusktr commented Jan 8, 2020

Ah, you're right!

From Node 13.1 combined with --experimental-modules to Node 13.2 without --experimental-modules the behavior changed, and now it works!

working example (in Node 13.2 or higher): https://github.com/trusktr/node-es-modules-alias-root-to-dist


I was assuming that it didn't work because the latest docs (13.6) still say the opposite in the Package Exports section:

In addition to defining an alias, subpaths not defined by "exports" will throw when an attempt is made to import them:

import submodule from 'es-module-package/private-module.js';
// Throws ERR_MODULE_NOT_FOUND

@trusktr
Copy link
Contributor

trusktr commented Jan 8, 2020

Maybe the docs weren't clear enough. I realized that if exports specifies a file, it hides other files. If exports specifies a directory, then all files in it can be accessed deeply (unlike pre Node 13.2).

Let me see if I can update wording to make it more clear.

EDIT: I'm not sure I understand it well enough to update wording yet, but after playing with it, the following exports config is working with regards to what people want above:

    "exports": {
        // this allows people to `import {...} from 'the-package'` directly
        // This replaces the `main` field, though the `main` field can be kept for backwards compatibility with Node versions that don't yet read the `exports` field.
        ".": "./dist/index.js",

        // this allows people to `import {...} from 'the-package/any/subfolder'
        // where `the-pacakge/dist/` contains `any/subfolder/`
        "./": "./dist/"
    },
    // The `main` field can be used as a fallback for older versions of Node:
    "main": "./dist/index.cjs",

@MylesBorins Yes indeed the issue can be closed. Thanks for showing that.

@okdecm
Copy link

okdecm commented Mar 8, 2020

@trusktr Your comment just saved me a lot of heartache and stress. Thank you for that explanation, this should be in the documentation!

@bmeck
Copy link
Member

bmeck commented Sep 30, 2020

closing per #14970 (comment)

@ghost
Copy link

ghost commented Sep 14, 2022

 "exports": {
        // this allows people to `import {...} from 'the-package'` directly
        // This replaces the `main` field, though the `main` field can be kept for backwards compatibility with Node versions that don't yet read the `exports` field.
        ".": "./dist/index.js",

        // this allows people to `import {...} from 'the-package/any/subfolder'
        // where `the-pacakge/dist/` contains `any/subfolder/`
        "./": "./dist/"
    },

I'm getting an warning saying

Property ./ is not allowed.

@sitatec
Copy link

sitatec commented Sep 17, 2022

@chathu-novade, use ./* instead

@mayank1513
Copy link

This approach does not work. At least with the turbo repo setup - trying to use this approach to export from dist forlder in this repo - https://github.com/react18-tools/turborepo-template/blob/main/lib/fork-me/package.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests