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

detect package manager and improve types #3847

Merged
merged 1 commit into from
Jul 7, 2022
Merged

Conversation

FredKSchott
Copy link
Member

Changes

  • Telemetry: Detect package manager so that we can better understand where Astro runs
  • Improve project info types
  • Improve debug output

Testing

  • Tested locally.

Docs

  • N/A

@changeset-bot
Copy link

changeset-bot bot commented Jul 7, 2022

🦋 Changeset detected

Latest commit: 47042e0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/telemetry Patch

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

@@ -150,6 +137,15 @@ export class AstroTelemetry {
context.anonymousId = `CI.${meta.ciName || 'UNKNOWN'}`;
}

if (this.debug.enabled) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this down here so that it would output all data collected, not just the specific event payload.

Copy link
Contributor

@bholmesdev bholmesdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking comment on which-pm, but this should give our telemetry a nice packadvantage 👍

@@ -34,11 +34,13 @@
"git-up": "^4.0.5",
"is-docker": "^3.0.0",
"is-wsl": "^2.2.0",
"node-fetch": "^3.2.5"
"node-fetch": "^3.2.5",
"which-pm-runs": "^1.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Callout that we have a which-pm helper in create-astro already! Pulled from Vite's create-vite package, and looks roughly the same as the which-pm-runs source code. Happy to pull in this package though given how small it is 🤷 Your call on which we'd prefer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mean preferred-pm, which I'd be happy to use but has an async-only API and this needs to be quick and sync. That's also more complex since it looks at your file system, reads up directory trees for different lockfiles, etc.

I actually wonder if create-astro should be moved to which-pm-runs, since actually don't want that data when running create-astro.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually wait, it looks like it's used in add, and not create-astro. That actually needs to rely on your file system and not just which pm you used to run command with, so we can keep as is.

create-astro is using a custom solution, which could probably be replaced with this to save us some maintainance.

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@github-actions github-actions bot added the pkg: create-astro Related to the `create-astro` package (scope) label Jul 7, 2022
@FredKSchott FredKSchott merged commit eedb32c into main Jul 7, 2022
@FredKSchott FredKSchott deleted the telemetry-update-2 branch July 7, 2022 18:12
@astrobot-houston astrobot-houston mentioned this pull request Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: create-astro Related to the `create-astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants