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

module: empty fallback and null exports as not exported #32838

Closed
wants to merge 2 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Apr 14, 2020

This PR includes two changes to exports edge cases:

  1. Empty array mappings ("./subpath": []) to throw ERR_PACKAGE_PATH_NOT_EXPORTED.
  2. null targets ("./subpath": null) as also throwing ERR_PACKAGE_PATH_NOT_EXPORTED.

This comes out of the discussion in jkrems/proposal-pkg-exports#46 where the feature is blacklisting subdirectories via eg:

{
  "exports"; {
    "./features/": "./dist/features/",
    "./features/private/": null
  }
}

Both of the above cases would then result in conditional object checks falling through (whereas the current ERR_INVALID_PACKAGE_TARGET for both does not).

There is likely still some discussion to be had here:

  • Currently empty array mappings are treated as invalid meaning that they always throw as a sort of package author validation step since there's no real reason to use them. Either error can in some way be seen as consistent though - [[]] will throw the same error regardless. So it likely is about what is best for usability and what will more likely match import maps.
  • null in import maps was originally proposed to match the semantics of [], so should likely follow the above.

Questions:

  • Do we want to support this use case?
  • Does it make sense to support it via null?
  • If so, should [] follow this same error?

I tend to think yes, but let's discuss in the meeting further.

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

@nodejs-github-bot nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label Apr 14, 2020
@guybedford guybedford added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Apr 14, 2020
@guybedford
Copy link
Contributor Author

/cc @nodejs/modules-active-members

Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! It feels more consistent.

@nodejs-github-bot
Copy link
Collaborator

@MylesBorins
Copy link
Contributor

From modules team discussion today it seems fine and "we should just do it"

@MylesBorins MylesBorins removed the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Apr 22, 2020
guybedford added a commit that referenced this pull request Apr 22, 2020
PR-URL: #32838
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@guybedford
Copy link
Contributor Author

Landed in e767ed0.

@guybedford guybedford closed this Apr 22, 2020
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
PR-URL: #32838
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Apr 27, 2020
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
PR-URL: #32838
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
targos pushed a commit that referenced this pull request Apr 30, 2020
PR-URL: #32838
Reviewed-By: Jan Krems <jan.krems@gmail.com>
targos pushed a commit that referenced this pull request May 13, 2020
PR-URL: #32838
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
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants