nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.6k stars 29.59k forks source link

Add `mainDir` to module resolution algorithm #14970

Closed davewasmer closed 4 years ago

davewasmer commented 7 years ago

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 commented 7 years ago

Some prior art: https://github.com/nodejs/node/issues/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.

davewasmer commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 6 years ago

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

igoodrich commented 6 years ago

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.

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 commented 6 years ago

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 commented 6 years ago

This would indeed VERY nice to have

lazeebee commented 5 years ago

Wouldn't this make conflicts with node_modules paths?

devsnek commented 5 years ago

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

nixxquality commented 5 years ago

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 commented 5 years ago

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:

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 commented 5 years ago

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 commented 5 years ago

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

wKich commented 5 years ago

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

devsnek commented 5 years ago

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

GongT commented 5 years ago

@devsnek I's ugly.

wKich commented 5 years ago

@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 commented 5 years ago

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

devsnek commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

guybedford commented 5 years ago

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 commented 5 years ago

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

https://github.com/lerna/lerna/issues/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. https://github.com/angular/material2/issues/4137 https://github.com/angular/angular-cli/issues/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 commented 5 years ago

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

BridgeAR commented 5 years ago

I agree with @targos and @guybedford that this could be solved at publishing time. The feature itself might not be trivial and would have to resolve a couple of things but that should be possible. I don't think it's something we should include in Node.js itself. It would otherwise complicate the module loading and that is not desirable.

https://github.com/sindresorhus/np seems a working solution for most people in the meanwhile.

For the given reasons I'll close this issue. It just seems the wrong place to implement this feature. If any collaborator disagrees, please feel free to reopen.

chyzwar commented 5 years ago

The whole thing is not about publishing!!!. If you would read the rationale presented by @davewasmer, @nfroidure, @igoodrich and me you would understand that the problem is about the common case where you expose more that one thing in the package.

Pre-publish is not solution because:

We all tried proposed solution and it sucks. I even written something simmilar to np and I know first hand that this is bad idea.

mehdikhody commented 5 years ago

Hi, Is there any package to help to do that ?

richturner commented 5 years ago

Indeed like others have mentioned when working in a monorepo scenario with transpilation (e.g. typescript) and packages are sym-linked (e.g. yarn workspaces) then you are working with the package in an unpublished state so nothing to do with publishing.

The disparity occurs when the transpiler outputs to a different location to the source files (e.g. src -> dist) and then imports need to be adapted (the main package.json entry solves this problem perfectly for a single file).

The only way to solve this currently is to get the transpiler to output to the source location during dev and for publishing output to a separate dir for package publishing....I like many others don't like polluting the source directory with transpiled files which is why a mainDir would help.

morganney commented 5 years ago

Doesn't seem necessary, even for monorepos with symlinked workspaces, just modify your development environment and build scripts.

packages
  dependency
    dist
    src // build outputs to "dist" so we don't pollute the source area
    package.json // main field is "./dist/index.js" for dev
  dependent
    package.json // has "dependency" workspace listed in its dependencies field

You want to publish dependency without forcing consumers to include dist in the require path? Just copy the packages/dependency/package.json over to packages/dependency/dist/package.json while renaming the main field as needed (maybe from ./dist/index.js over to index.js) and then publish from dist.

You don't want to adapt require paths for development? Just use an alias, for example something like webpack's resolve alias, or modify your require context with something like this or require.resolve.

Does it take a bit more work? Yes. Does it mean we have to consider changing a stable API? No.

borekb commented 5 years ago

Maybe the new exports field in package.json resolves this? I just learnt about it from https://github.com/nodejs/node/issues/21787#issuecomment-524709246, seems exactly like what we'd use.

chyzwar commented 5 years ago

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

  "exports": {
    "./": "./lib/"
  }
zakkudo commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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

trusktr commented 4 years ago

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 commented 4 years ago

deleted

trusktr commented 4 years ago

deleted

trusktr commented 4 years ago

@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 commented 4 years ago

@trusktr I'm a bit confused by https://github.com/nodejs/node/issues/14970#issuecomment-570036324

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 commented 4 years ago

Reopened for further discussion.

trusktr commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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

trusktr commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

bmeck commented 4 years ago

closing per https://github.com/nodejs/node/issues/14970#issuecomment-571887546

ghost commented 2 years ago
 "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.