Skip to content
This repository was archived by the owner on Apr 16, 2020. It is now read-only.

[Do not merge] doc: Add pkg-exports proposal to resolve spec #14

Closed
wants to merge 1 commit into from
Closed

[Do not merge] doc: Add pkg-exports proposal to resolve spec #14

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 31 additions & 4 deletions doc/api/esm.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,12 +179,39 @@ _ESM_RESOLVE_:
> 1. Assert: _packageSpecifier_ is a bare specifier.
> 1. If _packageSpecifier_ is a Node.js builtin module then,
> 1. Return _"node:${packageSpecifier}"_.
> 1. Set _package_ to the result of
> **FIND_PACKAGE_BOUNDARY**(_packageSpecifier_, _parentURL_).
> 1. Set _meta_ to the result of loading the resource at _package.metaURL_ as JSON.
> 1. Set _packagePath_ to the result of removing _package.name_
> from the beginning of _packageSpecifier_.
> 1. Set _directResolution_ to _meta.exports\[packagePath]_.
> 1. If _directResolution_ is a string,
> 1. Return the URL resolution of _directResolution_ relative to _package.metaURL_.
> 1. Set _directoryKeys_ to the list of _meta.exports_ keys that end with "/",
> sorted descending by length (longest key first).
> 1. For each _directoryKey_ in _directoryKeys_,
> 1. If _packagePath_ begins with _directoryKey_,
> 1. Set _directoryPath_ to the result of removing _directoryKey_ from the
> beginning of _packagePath_.
> 1. Set _directoryResolution_ to "${_meta.exports\[_directoryKey_]}".
> 1. Assert: _directoryResolution_ ends with "/".
> 1. Return the URL resolution of "${_directoryResolution_}${_directoryPath_}"
> relative to _package.metaURL_.
> 1. Throw a _Module Not Found_ error.

**FIND_PACKAGE_BOUNDARY**(_packageSpecifier_, _parentURL_)
> 1. While _parentURL_ contains a non-empty _pathname_,
> 1. Set _parentURL_ to the parent folder URL of _parentURL_.
> 1. Let _packageURL_ be the URL resolution of
> _"${parentURL}/node_modules/${packageSpecifier}"_.
> 1. If the file at _packageURL_ exists then,
> 1. Return _packageURL_.
> 1. Set _packageNameSegments_ to an empty list
> 1. While _packageNameSegments_ does not include all of _packageSpecifier_,
> 1. Append the next path segment of __packageSpecifier_
> to __packageNameSegments_.
> 1. Set _packageName_ to "${_packageNameSegments_.join("/")}"
> 1. Let _packageMetaURL_ be the URL resolution of
> _"${_parentURL_}/node_modules/${_packageName_}/package.json"_.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned when we discussed, I still worry a little about this in cases where packages do not have a package.json file, as in theory it should still be possible to resolve subpaths in these cases (for eg legacy package backwards compatibility)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case would be:

mkdir -p node_modules && touch node_modules/foo.mjs
esm-node -e 'import "foo.mjs"'

Is this something we should allow via import? It would continue to work for .js and via require for people who truly need it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also where node_modules/pkg/x.js exists without a node_modules/pkg/package.json I think? Or is that ok?

Copy link
Contributor Author

@jkrems jkrems Nov 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any package installed via npm/yarn would have a package.json afaik. Manually created package-less files in node_modules feel like a hack that exploited the previous design space to get a specific result. I don't believe too many people who know node would be able to understand why it's working (or expect to find it in a project).

If we do want to support this for power users, I believe it should be opt-in (e.g. via loader or config), not the default.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having this kind of manual public entrypoint specification probably has value for cjs just as much as esm, tbh. And for cjs, this would need to be opt-in, so it'd make sense for esm to be the same.

Copy link

@weswigham weswigham Nov 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's well and good, but I'd rather that not be tethered to esm just because it seems like a sneaky way to introduce changes without them obviously being breaks. And like I said and you agreed, this has equal value describing what people want to be able to accomplish with plain ole cjs - which leads me to the conclusion that this aughta be a core cjs loader feature that is replicated in the esm loader, rather than going the other way and trying to "backport" the feature to cjs in the future. Heck, it's an argument for designing explicitly with cjs in mind to ensure it works for it. And as an added bonus, if you propose this for cjs proper, it could start getting worked on being merged into core right away if people agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] it seems like a sneaky way to introduce changes without them obviously being breaks.

I think that's an overly negative way of portraying it. It's not about sneaking in changes, it's about being able to clean up the API. CJS has a lot of assumptions built in, e.g. how it's tied to a specific file system layout. I don't think it's realistic to remove those assumptions (or even just loosen them) but maybe I'm wrong.

From observing what people are doing in practice, I assume CJS as implemented in node core will be more and more a system that people just wholesale replace. Yes, landing features in core for CJS might be possible. But then we'd just have to reimplement it each replacement again until node-CJS becomes usable as-is. And I'd be (positively) surprised if node core would be prepared to take on the potential perf regression of a more flexible CJS system that still preserves full compatibility with all existing code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I'd be (positively) surprised if node core would be prepared to take on the potential perf regression of a more flexible CJS system that still preserves full compatibility with all existing code.

Can't you bill this as potentially improving perf by allowing the resolver to skip disk hits for fallback locations when exact mappings are provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is about the more general thing: Being able to not even hit the disk for resolution and storing packages outside of an on-disk hierarchy in node_modules. That is the reason why people replace the whole CJS system and/or shim around FS (see source of tink or yarn pnp). That implies adding hooks into the resolution pipeline which might affect out-of-the-box performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying the end result when using the npm or yarn resolver would be slower. It would be faster. But at the expense of using node without those resolvers. Which is what node core would realistically benchmark. If node core doesn't add those hooks, I'm not sure it's super relevant if it adds exports support because nothing would actually hit that code.

> 1. If a resource with content-type "application/json" exists
> at _packageMetaURL_,
> 1. Return { metaURL: _packageMetaURL_, name: _packageName_ }.
> 1. Throw a _Module Not Found_ error.

[Node.js EP for ES Modules]: https://github.com/nodejs/node-eps/blob/master/002-es-modules.md
Expand Down