-
-
Notifications
You must be signed in to change notification settings - Fork 54
feat(add): install prompt only if needed #696
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: d941540 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
const checkInstallNeeded = (atLeastVersion: string, version: string | undefined) => { | ||
if (!version) return true; | ||
const atLeastVersionStripped = atLeastVersion.replace('^', ''); | ||
const versionStripped = version.replace('^', ''); | ||
return !isVersionAtLeast(versionStripped, atLeastVersionStripped); | ||
}; |
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.
i think we may be doing too much with the inclusion of version checking. dependency versions can get very complicated with tons of edge-cases, so i'm not sure if this is really worth the effort. (note: we can get away with it with @types/node
since we're targeting a very specific dependency version that we'll always know the shape/type of. we don't really have that guarantee with other 3rd party deps)
perhaps we should just show the install prompt only if there are any dependencies to be installed? although, im not sure how useful that'd be since almost every add-on tends to add some sort of new dependency
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.
very complicated with tons of edge-cases
Don't tell me 😅, spent hours there before.
The idea is that checkInstallNeeded
(and isVersionAtLeast
under), do it's best for simple case, and if not will fallback with prompting. So I would say that it's helping "normal" cases and fallback to todays way.
I would totally see "Community Add-On" for scaffolding code (with a dep).
But you are already the second one saying that it might not worth the effort. (It's good to slow down my ideas from time to time :p)
I still think it's a good addition as the fallback is like today. But yes, it's a bit of code, maintenance, surface, ...
Closes: #687