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

feat: error codes & validate targets #6

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: error codes & validate targets #6

wants to merge 3 commits into from

Conversation

lukeed
Copy link
Owner

@lukeed lukeed commented Jan 23, 2021

Added error codes (matching Node.js' codes) – and retracted my stance on #5 for now – at least to see how it looks. This does, however raise the question of how directory targets should be handled. Node.js throws ERR_UNSUPPORTED_DIR_IMPORT but I think this is because of its file-system stage – and not because it's an invalid "exports" target.

While Node has this marked for deprecation, it's not actually deprecated yet.
I'm fairly certain this means that the pattern is still valid and may be in use by others? Specifically, this:

{
  "exports": {
    "./features/": "./features/", // <~ this
    "./": "./", // <~ NOT this – it's special
  }
}

I believe Vite relies on this?

Apologize for the pings, but would very much appreciate the sanity check here so that there can be agreement moving forward. /cc @developit @MylesBorins @paul-soporan @yyx990803 @FredKSchott

*/
function validate(name, entry, value) {
if (value[0] != '.' || value[1] != '/') throws('ERR_INVALID_PACKAGE_TARGET', `Invalid "${entry}" export in "${name}" package; targets must start with "./"`);
// if (value[value.length - 1] == '/') throws('ERR_UNSUPPORTED_DIR_IMPORT', `Invalid "${entry}" export in "${name}" package; targets must not resolve to a directory`);
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is where ERR_UNSUPPORTED_DIR_IMPORT would throw.

@codecov-io
Copy link

Codecov Report

Merging #6 (1bd82bb) into master (c6814c4) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master        #6   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           51        61   +10     
=========================================
+ Hits            51        61   +10     
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6814c4...1bd82bb. Read the comment docs.

@arcanis
Copy link

arcanis commented Feb 9, 2021

This does, however raise the question of how directory targets should be handled. Node.js throws ERR_UNSUPPORTED_DIR_IMPORT but I think this is because of its file-system stage – and not because it's an invalid "exports" target.

Imo the safest would be to be conservative and throw for now. Once someone opens up an issue asking for it it can be revisited based on the data they'll provide, but at least as a first iteration it's fine to restrict behaviors you're not sure about. Easier to implement a feature that used to throw than to remove one that used to work! 🙂

@ljharb
Copy link

ljharb commented Feb 9, 2021

Directory imports are "deprecated" but definitely still work, and resolution tools need to be able to use them, because authors will need to test on node versions that have them for a long time.

@lukeed
Copy link
Owner Author

lukeed commented Feb 10, 2021

I'll need to keep directory targets actually. It's still supported by Node.js – and it's supported by webpack (and I think Rollup, too)

I don't want this module tied to the file system – or pretend it is – since it's meant to work in browsers too so that tools like wmr, Vite, and Snowpack have the ability to use this in their runtime instead of being forced to offload the work to a server.

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

Successfully merging this pull request may close these issues.

4 participants