jkrems / proposal-pkg-exports

Proposal for Bare Module Specifier Resolution in node.js
MIT License
129 stars 14 forks source link

How to blacklist specific directories? #46

Open jaydenseric opened 4 years ago

jaydenseric commented 4 years ago

A common requirement is to expose an entire directory to the public, but not a sub-directory of internal helpers.

For example, with:

{
  "files": [
    "lib"
  ],
  "main": "lib",
  "exports": {
    ".": "./lib/index.js",
    "./lib/": "./lib/"
  }
}

How can you make ./lib/helpers/ private?

It's possible to move the helpers directory out of the lib directory to be at the same level, but sometimes this is not desirable. For example, graphql-react has helper files scoped under universal, server and test directories so that they can have seperate .babelrc.js files for transpilation per environment:

https://github.com/jaydenseric/graphql-react/tree/v10.0.0/src

Ideally the helper files could be gathered into helpers directories, and blacklisted in place. Moving the helpers out from under the scope of the .babelrc files would be a hassle, it would mean duplicating the Babel config files, so 3 becomes 6 with repetition.

Alternatively, the structure could be changed to graphql-react/universal/lib/foo.mjs and graphql-react/universal/helpers/bar.mjs, but inserting the extra /lib into the paths is a breaking change to the existing paths and looks ugly for consumers to write.

ljharb commented 4 years ago

Add a package.json inside lib with its own "exports".

jaydenseric commented 4 years ago

@ljharb not sure how that is any different to putting additional exports rules in the top level package.json?

ljharb commented 4 years ago

True, it's basically the same.

jaydenseric commented 4 years ago

Ok, here is one possible way to blacklist that seems to work:

{
  "name": "a",
  "main": "lib",
  "exports": {
    ".": "./lib/index.js",
    "./lib/": "./lib/",
    "./lib/helpers/": "./this-dir-doesnt-exist/"
  }
}

For ./this-dir-doesnt-exist/, it doesn't seem to matter if the directory truly doesn't exist, or if you put an empty directory there. The MODULE_NOT_FOUND error if you try to require('a/lib/helpers/index.js') is the same.

Is there a problem with doing this, will people understand that this fake path is on purpose, and not a mistake?

ljharb commented 4 years ago

Instead of a fake path, what if you do null or []?

jaydenseric commented 4 years ago

Good ideas, here's the results.

  1. null:

    internal/modules/cjs/loader.js:631
      throw new ERR_INVALID_PACKAGE_TARGET(
      ^
    
    Error [ERR_INVALID_PACKAGE_TARGET]: Invalid "exports" target null defined for './lib/helpers/' in the package config /Users/jaydenseric/Desktop/package-exports-test/node_modules/a/package.json
        at resolveExportsTarget (internal/modules/cjs/loader.js:631:9)
        at applyExports (internal/modules/cjs/loader.js:519:14)
        at resolveExports (internal/modules/cjs/loader.js:541:12)
        at Function.Module._findPath (internal/modules/cjs/loader.js:661:22)
        at Function.Module._resolveFilename (internal/modules/cjs/loader.js:963:27)
        at Function.Module._load (internal/modules/cjs/loader.js:859:27)
        at Module.require (internal/modules/cjs/loader.js:1036:19)
        at require (internal/modules/cjs/helpers.js:72:18)
        at Object.<anonymous> (/Users/jaydenseric/Desktop/package-exports-test/test.js:2:1)
        at Module._compile (internal/modules/cjs/loader.js:1147:30) {
      code: 'ERR_INVALID_PACKAGE_TARGET'
    }
  2. []:

    internal/modules/cjs/loader.js:587
      throw new ERR_INVALID_PACKAGE_TARGET(StringPrototypeSlice(baseUrl.pathname
      ^
    
    Error [ERR_INVALID_PACKAGE_TARGET]: Invalid "exports" target [] defined for './lib/helpers/' in the package config /Users/jaydenseric/Desktop/package-exports-test/node_modules/a/package.json
        at resolveExportsTarget (internal/modules/cjs/loader.js:587:13)
        at applyExports (internal/modules/cjs/loader.js:519:14)
        at resolveExports (internal/modules/cjs/loader.js:541:12)
        at Function.Module._findPath (internal/modules/cjs/loader.js:661:22)
        at Function.Module._resolveFilename (internal/modules/cjs/loader.js:963:27)
        at Function.Module._load (internal/modules/cjs/loader.js:859:27)
        at Module.require (internal/modules/cjs/loader.js:1036:19)
        at require (internal/modules/cjs/helpers.js:72:18)
        at Object.<anonymous> (/Users/jaydenseric/Desktop/package-exports-test/test.js:2:1)
        at Module._compile (internal/modules/cjs/loader.js:1147:30) {
      code: 'ERR_INVALID_PACKAGE_TARGET'
    }

Looks like they are both invalid :(

jaydenseric commented 4 years ago

I think null should be a valid value, to allow deliberate blacklisting.

ljharb commented 4 years ago

Right, but that means it throws - correctly - when you attempt to access the path?

jaydenseric commented 4 years ago

Sure it throws, but the ERR_INVALID_PACKAGE_TARGET error is from the perspective that the package exports is misconfigured, not that the user has imported something wrongly.

Even the fake path solution is not ideal in that regard, because the MODULE_NOT_FOUND error is actually different to the regular ERR_PACKAGE_PATH_NOT_EXPORTED error users would expect if they are trying import something private. They might look and see the file there in node_modules, and get frustrated because the file should be found.

coreyfarrell commented 4 years ago

Add a package.json inside lib with its own "exports".

To my knowledge "exports" is only applied from the package.json at the top-level of any package. I've experimented with putting package.json in sub-directories and was not able to get node.js to recognize it.

In node.js 13.10.0+ the following seems to produce the desired result:

  "name": "pkg1",
  "exports": {
    "./lib/": "./lib/",
    "./lib/helpers/": {}
  }

See https://github.com/coreyfarrell/pkg-exports for a demonstration of this, require('pkg1/lib/helpers/') gives the desired error in 13.10.0+ with code ERR_PACKAGE_PATH_NOT_EXPORTED. Earlier versions of node.js 13 give different but IMO still acceptable errors.

guybedford commented 4 years ago

Explicitly supporting null as indicating the ERR_PACKAGE_PATH_NOT_EXPORTED for trailing slash matches sounds like it would be a useful feature! I'd gladly approve a Node.js PR along these lines, and it should be relatively straightforward.

guybedford commented 4 years ago

Also, the empty [] should probably be giving a ERR_PACKAGE_PATH_NOT_EXPORTED as well, so perhaps that is something that should be fixed too.

jkrems commented 4 years ago

I think I'd prefer documenting {} as the canonical way of doing it. That way there's one pattern people have to learn and it'll hopefully easier to recognize in unfamiliar projects. I'm not sure null is fundamentally easier to understand than {} in this context.

Not going to block any PR or even discourage creating them. Just in terms of documentation, I think we should pick one. And if {} is already given valuable error messages, it seems nice to just go with it.

guybedford commented 4 years ago

I've created a PR for further discussion in https://github.com/nodejs/node/pull/32838.