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

modules: Package exports validations and fallbacks #28949

Closed
wants to merge 11 commits into from

Conversation

@guybedford
Copy link
Contributor

commented Aug 3, 2019

This clarifies and specifies the package "exports" proposal validations roughly in a way similar to as specified for import maps (except slightly more restrictive - no URLs or backtracking).

The validations implemented are:

  • Directory mappings must end in a trailing /
  • Backtracking below the package base is not permitted
  • If a user attempts to load a directory mapping below the package base, that is not permitted (pkg/map/../../below).
  • Targets starting with ../ or / are not permitted
  • Targets that are bare specifiers are not permitted
  • Targets that are URLs are not permitted
  • Targets cannot map into a node_modules path segment
  • Directory mappings into node_modules segments are not permitted

Instead of throwing on array targets, a very simple version of fallback arrays is added to allow for forwards compatibility in future, behaving just like import maps where the first valid match of the array is selected, based on the validation rules above.

The implementation is provided for both CJS and ESM, with associated specification changes for both as well.

Along with banning node_modules segments for exports, for consistency an associated addition to the package scope lookup algorithm is provided to avoid checking package scope package.json lookups through any node_modules path segment.

In addition a test has been included for the use of %2F in exports targets and user paths.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@guybedford guybedford requested a review from jkrems Aug 3, 2019

@guybedford guybedford force-pushed the guybedford:exports-validations branch from b158078 to 01f017b Aug 3, 2019

@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

@bmeck

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Should we be preventing nested node_modules paths?

foo => ./node_modules/bar?

@bmeck
Copy link
Member

left a comment

nits / question, seems fine if we just add tests to answer the question of inner packages and avoid breakage in future. i have no strong opinions on throwing vs allowing.

Show resolved Hide resolved lib/internal/modules/cjs/loader.js Outdated
Show resolved Hide resolved src/module_wrap.cc Outdated
Show resolved Hide resolved test/es-module/test-esm-exports.mjs
This is just a convention that works because `false`, just like `{}`, has no
iterable own properties.

Any invalid exports entries will be ignored. This includes exports not

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 3, 2019

Member

Should we perhaps warn on them, instead of silently ignoring them?

This comment has been minimized.

Copy link
@guybedford

guybedford Aug 4, 2019

Author Contributor

The problem with warnings in Node.js is that when you warn for behaviour of packages in node_modules, there isn't much the user can do. This is in contrast to the browser, where warnings are for developers not users (console = developers), wheras in Node.js the users get the console.

So if we just spit out these warnings and we have a future-compatibility path, then it's just going to be spam, so I'd be against that.

Perhaps we can explicitly make these debug messages though under NODE_DEBUG=exports or similar in a follow-on PR.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 4, 2019

Member

That’s a fair point, but it’s the same with that event emitter memory leak issue.

This comment has been minimized.

Copy link
@jkrems

jkrems Aug 5, 2019

Contributor

It's not quite the same as the memory leak warning because we expect that there are invalid entries and ignoring them on older versions is exactly what should be happening on those. I would compare it to warning on unknown package.json properties. It would catch mian: lib.js but it would be much more likely to warn on all kinds of stuff that is perfectly correct.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 5, 2019

Member

It’s the same in that it’s not actionable to the top level app, because it originates in an installed package.

The same is true for npm audit warnings.

I think this kind of warning is important for encouraging proper ecosystem behavior.

This comment has been minimized.

Copy link
@guybedford

guybedford Aug 6, 2019

Author Contributor

The latest commit I just added gives the following for invalid export resolutions -

eg -

{
  "exports": {
    "./x": "in:valid"
  }
}

import 'pkg/x' will then provide the error:

Error: Cannot resolve package exports target 'in:valid' matched for './x' in C:\Users\Guy\Projects\node\node_modules\pkg\package.json, imported from C:\Users\Guy\Projects\node\x.mjs
    at Loader.resolve [as _resolve] (internal/modules/esm/default_resolve.js:59:13)
    at Loader.resolve (internal/modules/esm/loader.js:73:33)
    at Loader.getModuleJob (internal/modules/esm/loader.js:149:40)
    at ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:43:40)
    at link (internal/modules/esm/module_job.js:42:36) {
  code: 'ERR_MODULE_NOT_FOUND'

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 6, 2019

Member

what about for any processing of that package.json? I’d expect a warning when the package is read, not just when that path is imported.

This comment has been minimized.

Copy link
@guybedford

guybedford Aug 6, 2019

Author Contributor

You could imagine a package that provides one export pkg/newfeature for modern Node.js, and another pkg/legacyfeature for legacy Node.js. If we validate all exports on load we get the same issue discussed above with warnings for things that aren't used.

As mentioned, I'd be open to a top-level flag for this kind of global validation, but I'd be against making it the default behaviour for the reason that it is simply entirely unactionable to the user - you can't even post a PR to the dependency package itself like other Node.js warnings for node_modules.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 6, 2019

Member

again, I’m talking about invalid ones, not nonexistent ones.

I think it’s appropriate to always warn when using a package whose package.json has any exports that can’t be used in my node version. The actionable item is “stop using that package” or “upgrade node”.

This comment has been minimized.

Copy link
@guybedford

guybedford Aug 7, 2019

Author Contributor

Along the lines of Jan's comment above, this is like saying that an API exporting:

exports.legacy = function () {
  // uses fs.oldAPI();
}
exports.modern = function () {
  // uses fs.newAPI();
}

should warn in legacy environments that the modern function exists just because it might call an undefined core library if you run pkg.modern().

There is nothing wrong with pkg.modern function existing if it isn't called in legacy environments. Similarly there should be nothing wrong with defining exports that only apply to modern use cases.

As I've mentioned, I'm happy to have a global validation mode as opt-in, but it shouldn't be the default for these reasons.

Show resolved Hide resolved lib/internal/modules/cjs/loader.js Outdated
Show resolved Hide resolved lib/internal/modules/cjs/loader.js Outdated
Show resolved Hide resolved src/module_wrap.cc Outdated
@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

Re importing from node_modules, I could get behind explicitly restricting that case as well, if others agree?

@bmeck

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

@guybedford easier to restrict for now and ease it later.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

Note that if we want to do this restriction, then we should also restrict the package boundary detection to not pass through node_modules as well. I can implement both together here.

Show resolved Hide resolved lib/internal/modules/cjs/loader.js
This is just a convention that works because `false`, just like `{}`, has no
iterable own properties.

Any invalid exports entries will be ignored. This includes exports not

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 4, 2019

Member

That’s a fair point, but it’s the same with that event emitter memory leak issue.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

Another interesting thing about blocking node_modules is that "exports": { "./": "./" } is no longer a noop as it opts-in to not being able to load from node_modules.

@jkrems

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

Ok I've implemented the node_modules restrictions, we can always relax this later too if need be, but it is difficult to add later so this seems the cautious route.

@guybedford guybedford requested a review from bmeck Aug 4, 2019

Show resolved Hide resolved doc/api/esm.md Outdated
Show resolved Hide resolved doc/api/esm.md Outdated
@bmeck
Copy link
Member

left a comment

minor request to ensure query and hash of URL don't get included in the path check.

@bmeck

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

@jkrems I think the problem here is we don't have an overall goal for what this feature is trying to aid. Is it trying to prevent package boundary crossing at all, is it trying to deal with package installation locations being unreliable, etc. I think the most conservative approach is the right one until we agree upon a goal so that we can relax as needed. For example, validating bundled dependencies could be checked via package.json#bundledDependencies but that expands in scope the implementation and how much of package.json node is tied to.

@jkrems

jkrems approved these changes Aug 5, 2019

Copy link
Contributor

left a comment

Very nice!

f. If either the key or exports[key] do not end with a slash (`/`),
throw "not found".
g. Return DIR/name/${exports[key]}${subpath.slice(key.length)}.
e. let RESOLVED_URL =

This comment has been minimized.

Copy link
@jkrems

jkrems Aug 5, 2019

Contributor

❤️

@guybedford guybedford force-pushed the guybedford:exports-validations branch from 677ee4e to 5e58810 Aug 6, 2019

@guybedford guybedford force-pushed the guybedford:exports-validations branch from e83c354 to 5a09519 Aug 6, 2019

guybedford added some commits Aug 6, 2019

@guybedford guybedford force-pushed the guybedford:exports-validations branch from 5a09519 to a7e3e76 Aug 6, 2019

@guybedford guybedford requested a review from bmeck Aug 6, 2019

@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

I've added a new commit here that refines the validation failure errors to include some contextual information so users can see how to correct it. We can hopefully continue to refine this over time.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

(Travis failure seems unrelated)

@jkrems

jkrems approved these changes Aug 6, 2019

Show resolved Hide resolved lib/internal/modules/cjs/loader.js

@jkrems jkrems added this to In progress in Support "exports" in package.json via automation Aug 6, 2019

@nodejs-github-bot

This comment has been minimized.

@jkrems jkrems moved this from In progress to Review in progress in Support "exports" in package.json Aug 6, 2019

@nodejs-github-bot

This comment has been minimized.

@bmeck

bmeck approved these changes Aug 6, 2019

Support "exports" in package.json automation moved this from Review in progress to Reviewer approved Aug 6, 2019

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@ljharb LGTY?

@ljharb

ljharb approved these changes Aug 8, 2019

Copy link
Member

left a comment

I still think we should be more aggressive on warning for any invalid export in any processed package.json - we can remove warnings later if needed, more easily than adding them.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

I'll aim to land this in the next day or so if no one beats me to it. Happy to continue the discussion on validations for potential follow-ups.

@nodejs-github-bot

This comment has been minimized.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Landed in 2103ae4.

@guybedford guybedford closed this Aug 12, 2019

Support "exports" in package.json automation moved this from Reviewer approved to Done Aug 12, 2019

@guybedford guybedford deleted the guybedford:exports-validations branch Aug 12, 2019

guybedford added a commit that referenced this pull request Aug 12, 2019

module: pkg exports validations and fallbacks
PR-URL: #28949
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.