Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

Index field proposal discussion #422

Closed
guybedford opened this issue Nov 3, 2019 · 15 comments
Closed

Index field proposal discussion #422

guybedford opened this issue Nov 3, 2019 · 15 comments

Comments

@guybedford
Copy link
Contributor

Creating this issue to discuss the index field PR created at nodejs/node#30239, as an alternative to the exports main sugar to support conditional exports.

See the PR there for further details. Marking modules agenda for Wednesday.

@ljharb
Copy link
Member

ljharb commented Nov 3, 2019

I’m surprised to see a PR for an undiscussed change; reading it I’m still unclear on why this is a good direction. I put a lot of weight on not adding new field names, and adding a third that node looks at seems like a big problem.

@guybedford
Copy link
Contributor Author

To be clear the feature is not intended to land before the meeting - the PR is exactly for tangible discussion.

@GeoffreyBooth
Copy link
Member

I have a few thoughts on this:

  1. The proposal for the field describes it as needed because the shorthand for "exports" (i.e. "exports" taking a string as opposed to an object) is confusing when it becomes capable of supporting conditional exports and therefore the object could be either a dictionary of paths or of conditions. This makes me think that we should just remove the "exports" shorthand form for now, at least until the dust settles on conditional exports. The verbose syntax is good enough for the initial release.

  2. I’m not a fan of removing exports["."]. I want to be able to define all my package exports in one place: I don’t want to need to define the root export in "index" and all others under "exports". I’m okay with "index" being yet another place to define the main entry point, after "main" and exports["."], assuming we can work out an order of precedence that makes sense to users. It feels broken to me that exports["./"] would work but exports["."] wouldn’t.

I have some sympathy for the argument that exports["."] is complicated for users, especially if we remove the shorthand. I think what we choose as a simpler alternative, though, depends on how popular we think packages with multiple exports (a.k.a. deep imports) are. If most packages have only one export, a.k.a. the main/bare specifier, then "index" makes sense: it provides a simpler syntax for the predominant use case. But if most packages provide multiple exports/deep imports, then "exports" will be used more often and "index" will provide a shorthand for a minority use case. I just don’t know which way most users land.

I also don’t think exports["."] is that difficult, that it necessarily needs a simpler alternative. We already have npm init to generate a package.json interactively, I’m sure it could be updated to generate exports["."] along with the "main" it creates now. We can also wait for user feedback on the verbose syntax before deciding that alternatives are necessary.

@guybedford
Copy link
Contributor Author

guybedford commented Nov 6, 2019

Thanks for the considered response here @GeoffreyBooth.

You raised the point that if most packages only have one specifier then index makes sense. To try clarify this I performed the following analysis:

  • Downloaded the top 100 npm packages
  • Of these packages, analyzed all their require() calls across all files in the package.
  • This resulted in 1427 unique dependency packages being collectively required (ie 1427 npm packages)
  • Of these 1427 dependency packages, only 109 packages had some importer from all the other packages that included a subpath require.

That is, we can reliably say for this ~1500 package dataset, that less than 10% of packages are using subpath requires.

@guybedford
Copy link
Contributor Author

(no I did not put that together in an hour... it just so happened I was working on the same problem analysis problems today anyway :P)

@ljharb
Copy link
Member

ljharb commented Nov 6, 2019

This doesn't include any consumption by apps, or projects not published to npm or github - and the usage of "exports" means that multiple entry points will likely be explicitly requested more often, as users lose the ability to implicitly access them when authors add the mechanism to their packages.

I don't think we should be basing this on how many people use deep entry points as much as based on whether we're trying to push "one entry point" onto the ecosystem - and I don't know why we'd want to do that.

@guybedford
Copy link
Contributor Author

guybedford commented Nov 6, 2019

@ljharb the observation here is simply that over 90% of packages only expose a main entry point to consumers. The analysis covers many uses of require of each package - the analysis is not of the top 100 packages, it is of the 1400 dependencies and the ways they are loaded within the graph of the top 100 packages, which means every analysis is based on many usages to get to these numbers.

@GeoffreyBooth
Copy link
Member

I have no data to back this up, but my anecdotal experience is that most packages probably only expect users to use a root export. Packages where deep imports are documented and encouraged, like 'lodash/shuffle' or 'aws-sdk/client/s3', are the exception rather than the rule.

This might change with the existence of "exports", as that would provide a machine-readable way of discovering deep imports that an IDE could prompt users to use; and deep imports should be encouraged, as the more they’re used the less overall code is loaded for an app and therefore the faster the app performs. But I think that most packages just aren’t designed as severable modules that can be used independently. I think design patterns will take a long time to adjust.

@ljharb
Copy link
Member

ljharb commented Nov 6, 2019

@guybedford that's not "only expose", that's "90% of packages are only consumed via a main entry point by other public packages". Every package exposes the same number of entry points as "files in the package".

@jkrems
Copy link
Contributor

jkrems commented Nov 6, 2019

@ljharb If you had the choice, which one would you prefer:

  1. Single conditional export can be put in a new top-level field index.
  2. Single conditional export can be put into exports under a key ..
  3. Single conditional export can be put into exports and there's a heuristic ("the keys don't start with .") to tell it apart.
  4. Single conditional export can be put into exports, wrapped in the array fallback notation.
  5. We allow conditional export syntax in main. But if you want to use it, you'd drop support for older runtimes that don't support it. Note: This wouldn't necessarily replace . in exports so it would be a choice to drop support or not.

Afaict those are the general options.

@ljharb
Copy link
Member

ljharb commented Nov 6, 2019

5 would be the worst.

I’m not sure why we’d be limited to a single conditional export, but if you mean in the case where there’s only the conceptual “main” and no additional entry points, I’d prefer 2, the current feature as i understand it.

3 is confusing since relative references should start with . always (unlike main/bin, and it’s confusing that main/bin alone works that way).

I don’t understand why it would be useful to require array notation for any use case that doesn’t involve fallbacks, eg, 4.

The proposal here is to change from 2 to 1, and i don’t think that it’s a good thing to:

  1. Add a new top level field that can be avoided
  2. privilege the use case for “a single export”, since the predominance of that usage pattern is why we all have to bear the tooling cost if treeshaking in the first place
  3. hide away the mental model that packages can have N entry points (just like Modules can)

@GeoffreyBooth
Copy link
Member

I’m not sure why we’d be limited to a single conditional export

I think what @jkrems meant was “for packages that want to provide only a single conditional export,” i.e. most packages today. They wouldn’t be limited to it, it’s just package author’s default design pattern (at least for now).

@ljharb
Copy link
Member

ljharb commented Nov 6, 2019

Thanks for clarifying. To me, I think that adding "index" encourages people to be unaware that multiple entry points are possible, and that's not the mental model we should be encouraging.

@guybedford guybedford removed the modules-agenda To be discussed in a meeting label Nov 6, 2019
@guybedford
Copy link
Contributor Author

I've updated the conditional exports proposal as described in nodejs/node#29978 (comment), based on feedback from this proposal.

@guybedford
Copy link
Contributor Author

The conditional exports sugar as discussed for this proposal was implemented into the conditional exports PR which landed on master. A new error is thrown for ambiguous exports objects that have both dot and non-dot-starting properties. Please do try it out!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants