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.

Sign up for GitHub

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

Jump to bottom

fix: temporarily disable telemetry on windows #5899

Merged
merged 4 commits into from
Jul 12, 2022
Merged

fix: temporarily disable telemetry on windows #5899

merged 4 commits into from
Jul 12, 2022

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Jul 11, 2022

"Closes" #5757, #5728. This isn't a fix, but the behavior is annoying enough that we should've disabled it a long time ago. (So we're disabling it now!)

At first I thought about making these changes in @redwoodjs/cli, but doing it in @redwoodjs/telemetryended up being simpler.

@jtoar jtoar added the release:fix This PR is a fix label Jul 11, 2022
@jtoar jtoar requested a review from dac09 July 11, 2022 08:30
@jtoar jtoar self-assigned this Jul 11, 2022
@netlify
Copy link

netlify bot commented Jul 11, 2022 •
edited
Loading

✅ Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit dda41cf
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62cd830a81a2bb0008627323

@jtoar jtoar linked an issue Jul 11, 2022 that may be closed by this pull request
Weird Issue with Windows 11 cmd #5757
Closed
@dac09 dac09 enabled auto-merge (squash) July 11, 2022 15:41
@dac09 dac09 mentioned this pull request Jul 11, 2022
Closed
@dac09 dac09 self-assigned this Jul 11, 2022
@dac09
Copy link
Contributor

dac09 commented Jul 11, 2022

Created this issue so we don't lose track. #5901

@dac09 dac09 disabled auto-merge July 11, 2022 19:39
@dac09 dac09 changed the title Temporarily disable telemetry on Windows fix: temporarily disable telemetry on windows Jul 11, 2022
dac09
dac09 reviewed Jul 11, 2022
View reviewed changes
packages/telemetry/src/telemetry.ts
@@ -1,4 +1,5 @@
import { spawn } from 'child_process'
import os from 'node:os'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be careful these - I'm not sure why... but I've found there are some cases where it throws errors (on Windows... if I remember correctly) - and other places the same imports didn't.

@dac09 dac09 closed this Jul 11, 2022
@dac09 dac09 reopened this Jul 11, 2022
@dac09
Copy link
Contributor

dac09 commented Jul 11, 2022

@jtoar - I can't the CI tests to pass ... I wonder if something is actually breaking after this change?

@jtoar
Copy link
Contributor Author

jtoar commented Jul 12, 2022

@dac09 I think CI is just a little flaky sometimes. I just gave it a kick and it's all green now. Here's the latest run: https://github.com/redwoodjs/redwood/actions/runs/2655619534.

@dac09 dac09 enabled auto-merge (squash) July 12, 2022 14:19
@dac09 dac09 merged commit 1ae8218 into main Jul 12, 2022
@dac09 dac09 deleted the ds-temporarily-disable-telemetry-on-windows branch July 12, 2022 15:13
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jul 12, 2022
dac09 added a commit to dthyresson/redwood that referenced this pull request Jul 12, 2022
@dac09
Merge branch 'dt-fastify-config-plugins' of https://github.com/dthyre…
abcb3c7 
…sson/redwood into dt-fastify-config-plugins

* 'dt-fastify-config-plugins' of https://github.com/dthyresson/redwood:
  fix: temporarily disable telemetry on windows (redwoodjs#5899)
  fix(deps): update dependency dotenv-webpack to v7.1.1 (redwoodjs#5909)
  fix(deps): update dependency dotenv-defaults to v5.0.2 (redwoodjs#5908)
  fix(deps): update dependency webpack-dev-server to v4.9.3 (redwoodjs#5906)
  fix(deps): update dependency core-js to v3.23.4 (redwoodjs#5905)
  Fix single-character typo (redwoodjs#5904)
@dac09
Copy link
Contributor

dac09 commented Jul 12, 2022

@jtoar as I mentioned in this PR - looks like the fancy node import causes issues.

PS C:\Users\dac09\dev\telemetry-test> yarn rw info
yarn run v1.22.11
$ C:\Users\dac09\dev\telemetry-test\node_modules\.bin\rw info
internal/modules/cjs/loader.js:892
  throw err;
  ^

Error: Cannot find module 'node:os'
Require stack:
- C:\Users\dac09\dev\telemetry-test\node_modules\@redwoodjs\telemetry\dist\telemetry.js
- C:\Users\dac09\dev\telemetry-test\node_modules\@redwoodjs\telemetry\dist\index.js
- C:\Users\dac09\dev\telemetry-test\node_modules\@redwoodjs\cli\dist\index.js
- C:\Users\dac09\dev\telemetry-test\node_modules\@redwoodjs\cli\package.json
-  ```

@dac09 dac09 mentioned this pull request Jul 12, 2022
Merged
dac09 added a commit to dac09/redwood that referenced this pull request Jul 12, 2022
@dac09
Merge branch 'main' of github.com:redwoodjs/redwood into feat/ts-stri…
346f0b1 
…ctmode-gen

* 'main' of github.com:redwoodjs/redwood: (22 commits)
  Remove "node:os" import from telemetry (redwoodjs#5914)
  fix: temporarily disable telemetry on windows (redwoodjs#5899)
  fix(deps): update dependency dotenv-webpack to v7.1.1 (redwoodjs#5909)
  fix(deps): update dependency dotenv-defaults to v5.0.2 (redwoodjs#5908)
  fix(deps): update dependency webpack-dev-server to v4.9.3 (redwoodjs#5906)
  fix(deps): update dependency core-js to v3.23.4 (redwoodjs#5905)
  Fix single-character typo (redwoodjs#5904)
  chore(deps): update dependency esbuild to v0.14.49 (redwoodjs#5898)
  Gitpod hot reload fix (redwoodjs#5888)
  fix(deps): update dependency @types/aws-lambda to v8.10.101 (redwoodjs#5894)
  fix(deps): update dependency @testing-library/user-event to v14.2.1 (redwoodjs#5893)
  fix(deps): update dependency @testing-library/react-hooks to v8.0.1 (redwoodjs#5892)
  chore(deps): update dependency typescript to v4.7.4 (redwoodjs#5886)
  feat: add noire-munich to core team (redwoodjs#5891)
  Fix file structure display (redwoodjs#5889)
  Make Form generic for better TS support (redwoodjs#5867)
  chore(deps): update dependency npm-packlist to v5.1.1 (redwoodjs#5885)
  Bump framer motion version to 6.* as recommended (redwoodjs#5870)
  chore(deps): update dependency nodemon to v2.0.19 (redwoodjs#5884)
  chore(deps): update dependency firebase to v9.8.4 (redwoodjs#5879)
  ...
jtoar added a commit that referenced this pull request Jul 13, 2022
@jtoar @dac09
fix: temporarily disable telemetry on windows (#5899)
0ea00be 
Co-authored-by: Daniel Choudhury <dannychoudhury@gmail.com>
@jtoar jtoar modified the milestones: next-release, v2.1.0 Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dac09 dac09 dac09 approved these changes

Assignees

@dac09 dac09

@jtoar jtoar

Labels
release:fix This PR is a fix
Projects
Main
Status: Archived
Milestone
v2.1.0
Development

Successfully merging this pull request may close these issues.

Weird Issue with Windows 11 cmd
2 participants
@jtoar @dac09