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

Document validations and fallbacks in spec #39

Merged
merged 3 commits into from Aug 14, 2019

Conversation

@guybedford
Copy link
Contributor

commented Aug 12, 2019

Now that this has been merged into Node.js core, it would be good to document the exact validations and package fallback mechanism.

Feedback welcome.

@@ -50,7 +52,8 @@ Here’s a complete `package.json` example, for a hypothetical module named `@mo
"exports": {
"./": "./src/util/",
"./timezones/": "./data/timezones/",
"./timezones/utc": "./data/timezones/utc/index.mjs"
"./timezones/utc": "./data/timezones/utc/index.mjs",
"./core-polyfill": ["std:core-module", "./core-polyfill"]

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 12, 2019

Contributor

we should probably also have an example that uses an optional dep - like chokidar or something - since that’s currently a common use case for file-watching utilities.

This comment has been minimized.

Copy link
@jkrems

jkrems Aug 12, 2019

Owner

I think that's its own discussion, worth a separate issue. Right now we only do static validation and won't try to fetch all candidates, in series. Given that a package manager could know at install-time that an optional dependency wasn't installed, it feels something better left to the package manager.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 12, 2019

Contributor

Are you suggesting that at install time, the package manager would rewrite an observable part of package.json? Because i think that’d be terrible and I’d advocate strongly against any package manager ever doing that.

I’d expect it to function in the same way as std:whatever - if it’s resolveable, grab it, else move to the next.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 12, 2019

Contributor

also there’s no fetching; in node all deps are present on disk.

This comment has been minimized.

Copy link
@jkrems

jkrems Aug 12, 2019

Owner

Disk access is fetching, lower latency but still fetching. And since its IO, it can be exposed to all the weirdness that IO brings. Access errors, temporary failures, race conditions in changing contents, ...

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 12, 2019

Contributor

I don't think it makes sense to privilege builtin modules over userland ones; if we're not going to validate the presence of userland ones at this stage, we shouldn't be validating the presence of anything.

This comment has been minimized.

Copy link
@jkrems

jkrems Aug 12, 2019

Owner

To nitpick: We're not validating the presence of an implementation but the fact that we recognize this kind of URL. I'd also assume that we ignore data: URLs with unsupported encodings or network protocols that we don't support like ftp:.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 13, 2019

Contributor

in that case, makes perfect sense to me for an early validation error.

This comment has been minimized.

Copy link
@guybedford

guybedford Aug 13, 2019

Author Contributor

Is there any further action to be taken on this one?

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 13, 2019

Contributor

i still think it’d be helpful to show an actual real world example with a common optional dep like chokidar, alongside this one.

Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved README.md Outdated
@jkrems

jkrems approved these changes Aug 12, 2019

Copy link
Owner

left a comment

Thanks for updating this!

guybedford and others added some commits Aug 13, 2019

Update README.md
Co-Authored-By: Jordan Harband <ljharb@gmail.com>
Update README.md
Co-Authored-By: Jordan Harband <ljharb@gmail.com>

@jkrems jkrems merged commit fe6ccf4 into jkrems:master Aug 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.