feat(cli): [STENCIL-12] Adding Telemetry and CLI features. #2964
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| import { getCompilerSystem, getLogger, getStencilCLIConfig } from './state/stencil-cli-config'; | ||
| import { checkTelemetry, disableTelemetry, enableTelemetry } from './telemetry/telemetry'; | ||
|
|
||
| export const taskTelemetry = async () => { | ||
| const logger = getLogger(); | ||
| const prompt = logger.dim(getCompilerSystem().details.platform === 'windows' ? '>' : '$'); | ||
| const isEnabling = getStencilCLIConfig().flags.args.includes('on'); | ||
| const isDisabling = getStencilCLIConfig().flags.args.includes('off'); | ||
|
Comment on lines
+5
to
+8
There was a problem hiding this comment. Similar to my comment above, I'm not sure what the appropriate interface to use is here - should the There was a problem hiding this comment.
There was a problem hiding this comment. To me it seems like there's two ways to get a a logger instance: As a user of the singleton, it's not exactly clear to me if there's a preferred way to get the logger without reading the code that the former is a wrapper around the latter. The ergonomics of pulling data off the singleton seems a little inconsistent to me - for some fields I need to call That said, I'm not sure adding a helper function for each private method necessarily helps here. It gives us consistency/parity where there's a helper function for each private field that we expose via an accessor, but it doesn't answer the "which should I use?" question. That's all a long winded way of saying "I think this is fine as is for now, because I don't have a great solution to offer up ATM". It's something we should be cognizant of as we evolve this class, but no action required ATM There was a problem hiding this comment. Generally you'll see me throughout this PR use the helper functions, until I need to update the value, at which point I'll use:
And the helper methods pretty much only try to help compensate line length. Don't worry, I'm not married to this interaction, but at a minimum the singleton definitely helps. We could get rid of the helper functions quickly, if you'd like me to do that let me know, I'm happy to! |
||
| const INFORMATION = `Opt in or our of telemetry. Information about the data we collect is available on our website: ${logger.bold( | ||
| 'https://stenciljs.com/telemetry', | ||
| )}`; | ||
| const THANK_YOU = `Thank you for helping to make Stencil better! 💖`; | ||
| const ENABLED_MESSAGE = `${logger.green('Enabled')}. ${THANK_YOU}\n\n`; | ||
| const DISABLED_MESSAGE = `${logger.red('Disabled')}\n\n`; | ||
| const hasTelemetry = await checkTelemetry(); | ||
|
|
||
| if (isEnabling) { | ||
| const result = await enableTelemetry(); | ||
| result | ||
| ? console.log(`\n ${logger.bold('Telemetry is now ') + ENABLED_MESSAGE}`) | ||
| : console.log(`Something went wrong when enabling Telemetry.`); | ||
| return; | ||
| } | ||
|
|
||
| if (isDisabling) { | ||
| const result = await disableTelemetry(); | ||
| result | ||
| ? console.log(`\n ${logger.bold('Telemetry is now ') + DISABLED_MESSAGE}`) | ||
| : console.log(`Something went wrong when disabling Telemetry.`); | ||
| return; | ||
| } | ||
|
|
||
| console.log(` ${logger.bold('Telemetry:')} ${logger.dim(INFORMATION)}`); | ||
|
|
||
| console.log(`\n ${logger.bold('Status')}: ${hasTelemetry ? ENABLED_MESSAGE : DISABLED_MESSAGE}`); | ||
|
|
||
| console.log(` ${prompt} ${logger.green('stencil telemetry [off|on]')} | ||
|
|
||
| ${logger.cyan('off')} ${logger.dim('.............')} Disable sharing anonymous usage data | ||
| ${logger.cyan('on')} ${logger.dim('..............')} Enable sharing anonymous usage data | ||
| `); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import { isInteractive } from './helpers'; | ||
| import { checkTelemetry } from './telemetry'; | ||
|
|
||
| /** | ||
| * Used to determine if tracking should occur. | ||
| * @param ci whether or not the process is running in a Continuous Integration (CI) environment | ||
| * @returns true if telemetry should be sent, false otherwise | ||
| */ | ||
| export async function shouldTrack(ci?: boolean) { | ||
| return !ci && isInteractive() && (await checkTelemetry()); | ||
| } |
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.
Can we please add tests on the return value now that we're returning a primitive? We should be able to assert that we return
truewhenwriteConfigis successful, andfalsewhen it failsThere 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.
Oh, great point! Yes.
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.
@splitinfinities did we get this under test?
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.
Is this adequate? https://github.com/ionic-team/stencil/pull/2964/files#diff-a2d702e7c67979d0440b8322026a5249d8206bbeaf3e73f7c84df87cfdd2d07cR69
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 is fine ATM - normally I'd like us to get a little more granular there with respect to what we're testing (calling the
ionic-configexposed functions directly) and splitting the tests out based on the assertion, but I think we can leave that for a future day