-
Notifications
You must be signed in to change notification settings - Fork 159
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: add support for URL in "packageManager"
#359
Conversation
This PR still lacks a protection for the built-in package manager (Corepack should not silently use a non-official Yarn/PNPM/npm version, unless there's some env variable that explicitly opt-into that) |
I'm sure how to handle the |
This comment was marked as off-topic.
This comment was marked as off-topic.
sources/corepackUtils.ts
Outdated
} | ||
|
||
export function isSupportedPackageManagerLocator(locator: Locator): locator is SupportedPackageManagerLocator { | ||
return !locator.isURL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this isURL
property? Why not just do a URL.canParse(locator.reference)
like we do with descriptors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's to avoid calling URL.canParse
more than one – it's a way to cache the value rather than re-compute it every time we need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That feels a little unnecessary imo, we're only going to make a single such check per command, I doubt it'd have a significant perf impact compared to the complexity cost.
I'd also tend to just check whether the string startsWith('https://')
rather than validate the full correctness - validating everything feels a little too susceptible to typos (what if I add a character that makes the string an invalid url? should it go in the semver path?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if I add a character that makes the string an invalid url? should it go in the semver path?
In all likeliness, an invalid URL due to a typo won't be a valid semver version either, so I don't think it matters.
validating everything feels a little too susceptible to typos
Well we can only work with well formed reference (either semver or URL), so I'm not sure where you going with that, I don't see how we could be forgiving, we need full correctness.
That feels a little unnecessary imo, we're only going to make a single such check per command, I doubt it'd have a significant perf impact compared to the complexity cost.
I feel like I'm missing something, I don't see how it can be simplified (except maybe by using URL
instances) – because otherwise TS cannot differenciate between the URL-based spec and the semver-based ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because otherwise TS cannot differenciate between the URL-based spec and the semver-based ones.
Why does it need to differenciate? That's the same thing we do in Yarn: we treat all ranges the same (as string), and we just branch their behaviour by runtime pattern matching. I don't see what we gain by using strict typing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need a fallback / polyfill for URL.canParse
or drop support for some Node.js versions.
https://nodejs.org/docs/latest-v21.x/api/url.html#urlcanparseinput-base specifies that it was added in v19.9.0, v18.17.0
and our engines.node
specifies that we support >=18.17.1
.
I think we dropped support for Node.js 19.x when it went EOL a while back, although I reckon it wouldn't hurt to specify that in the engines field. |
Keep in mind that's a breaking change, could you do it in a separate PR? |
Fixes: #354