-
Notifications
You must be signed in to change notification settings - Fork 358
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: allow frameworks to detect version of CLI that's being used #6226
Conversation
📊 Benchmark resultsComparing with 10d470d
|
@@ -606,7 +609,8 @@ export default class BaseCommand extends Command { | |||
token, | |||
...apiUrlOpts, | |||
}) | |||
const { buildDir, config, configPath, repositoryRoot, siteInfo } = cachedConfig | |||
const { buildDir, config, configPath, env, repositoryRoot, siteInfo } = cachedConfig | |||
env.NETLIFY_CLI_VERSION = { sources: ['internal'], value: netlifyCliVersion } |
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.
This is the main functional change in this PR.
I don't have strong feelings around the name of the var - maybe NETLIFY_CLI
would be better? We already inject NETLIFY_LOCAL=true
. We could reuse that, but I imagine some consumers are depending on its value being "true"
.
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 this var name is just fine - clear is best imo (even if it's wordy :D )
Oops, looks like this didn't auto-merge yet! Resolved the conflicts, would love another stamp :) |
And now the tests are failing :/ I swear they worked locally! |
Whoohoo, tests are passing! Would appreciate another approval in the new year :D |
@@ -26,6 +26,7 @@ import { | |||
exit, | |||
getToken, | |||
log, | |||
netlifyCliVersion, |
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.
nit: This name feels a bit redundant and overly verbose to me, since everything here pertains to the Netlify CLI and there's no need to make that explicit with a prefix. Could we use something like version
?
// @ts-expect-error TS(2571) FIXME: Object is of type 'unknown'. | ||
Object.entries(environment).filter(([, variable]) => variable.sources[0] !== 'general'), | ||
Object.entries(environment).filter( | ||
// @ts-expect-error TS(2571) FIXME: Object is of type 'unknown'. |
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 would it take to get rid of this?
Addresses https://linear.app/netlify/issue/COM-189/give-frameworks-a-way-to-detect-version-of-netlify-build-cli-thats.
This PR gives frameworks + framework adapters a way to detect the version of the Netlify CLI that it's called through. The Astro adapter, for example, can use this to detect if the CLI is at least on 17.8.x - else it can choose to fail the local dev, because not all features that it depends on are present on that version. It can then prompt the user to update their CLI.