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

chore(gatsby-telemetry): Migrate package to TS #22532

Closed
wants to merge 6 commits into from
Closed

chore(gatsby-telemetry): Migrate package to TS #22532

wants to merge 6 commits into from

Conversation

Eyas
Copy link
Contributor

@Eyas Eyas commented Mar 24, 2020

Description

This PR migrates packages/gatsby-telemetry/ to TypeScript.

Related Issues

Part of #21995.

Note that this exposed #22531, which will be handled in a separate PR.

@Eyas Eyas requested a review from a team as a code owner March 24, 2020 04:53
@Eyas Eyas force-pushed the ts/gatsby-telemetry branch from 8010110 to 36bc9ea Compare March 24, 2020 15:03
@Eyas Eyas mentioned this pull request Mar 25, 2020
Closed
@Eyas Eyas force-pushed the ts/gatsby-telemetry branch from 36bc9ea to 7de0f30 Compare March 25, 2020 04:48
@Eyas Eyas force-pushed the ts/gatsby-telemetry branch from 878d1fd to df30e65 Compare March 25, 2020 20:05
@sunecosuri sunecosuri mentioned this pull request Mar 30, 2020
Closed
@pieh
Copy link
Contributor

pieh commented Mar 31, 2020

I merged #22613 (the isTrackingEnabled PR that changes isTrackingEnabled signature to be function returning boolean (instead of another function), so there will need to be updates in this PR

@Eyas Eyas force-pushed the ts/gatsby-telemetry branch from 4878151 to 8bfba95 Compare March 31, 2020 14:32
@Eyas
Copy link
Contributor Author

Eyas commented Mar 31, 2020

Thanks. @pieh @LekoArts I just rebased and pushed. Can you take a look?

@Eyas Eyas force-pushed the ts/gatsby-telemetry branch from 8bfba95 to 1eda61a Compare May 27, 2020 01:29
@Eyas
Copy link
Contributor Author

Eyas commented May 27, 2020

FWIW this is updated now.

@Eyas Eyas added the status: needs core review Currently awaiting review from Core team member label May 28, 2020
@Eyas Eyas requested review from LekoArts and blainekasten May 28, 2020 19:45
Eyas and others added 6 commits June 9, 2020 15:46
@Eyas
Rename-only commit as part of TS migration.
d0f5071 
Helps PR view know that all these files have been renamed. Since some of
of the later changes in later commits are substantial, git doesn't
recognize the valid renamed .ts files otherwise.
@Eyas
chore(gatsby-telemetry): Migrate package to TS
cdb883d
@Eyas
gatsby-telemetry: Generate typegen.
c4ba4da
@Eyas
Fix some test cases.
7540306 
- Don't output __tests__ which jest thinks are cases
- properly import telemetry.
@Eyas
Fix postinstall script.
cd667b5
@Eyas
Better types for users of gastby-telemetry.
9cc5013
@Eyas Eyas force-pushed the ts/gatsby-telemetry branch from eca560c to 9cc5013 Compare June 9, 2020 19:55
blainekasten
blainekasten reviewed Jun 10, 2020
View reviewed changes
Copy link
Contributor

@blainekasten blainekasten left a 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 looking really good. Before merging this though I think there are a lot of improvements we can make to the amount of unknown in there. For example, looking at the decorateAll method in telemetry.ts, i searched through usages and it looks like the type should be something more like Record<string, string>

Let's look through uses and see if we can improve the unknowns to be more in line with what is used.

@Eyas
Copy link
Contributor Author

Eyas commented Jun 12, 2020

Record<string, string> seems right for default tags (deocrateAll), though not all tags, e.g. tags can contain {error: IErrorV1 | IErrorV1[], errorV2: IErrorV2 }.

I'll leave tags as unknown since they seem be able to include arbitrary key/value pairs.

@LekoArts
Copy link
Contributor

Thanks so much for the PR and sorry that we didn't get to merge it earlier! In the meantime a couple of other PRs changed the files bit by bit so that telemetry is actually done with the migration now!

@LekoArts LekoArts closed this Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blainekasten blainekasten blainekasten left review comments

@LekoArts LekoArts Awaiting requested review from LekoArts

Assignees
No one assigned
Labels
status: needs core review Currently awaiting review from Core team member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Eyas @pieh @LekoArts @blainekasten @MichaelDeBoey