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

TS migrate: victory-scatter #2394

Merged
merged 2 commits into from
Aug 3, 2022
Merged

TS migrate: victory-scatter #2394

merged 2 commits into from
Aug 3, 2022

Conversation

sblinde
Copy link
Contributor

@sblinde sblinde commented Jul 29, 2022

Description

This PR migrates the victory-scatter package to TypeScript!

@sblinde sblinde mentioned this pull request Jul 29, 2022
30 tasks
@sblinde sblinde self-assigned this Jul 29, 2022
@sblinde sblinde added the Type: TypeScript Issues related to typescript or type definitions label Jul 29, 2022
Copy link
Contributor

@gksander gksander left a comment

Choose a reason for hiding this comment

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

Looks good!

One thing I wanted to point out is the use of !, the non-null assertion operator. This TS operator basically says, "I'm telling you, the compiler, that I know this value isn't undefined or null". It's basically a way of telling TS to shut up a bit. For test files, which is where you have it used here, this isn't really a big deal. In application code, I always advise against this operator – as it waters down the usefulness of TS! It's generally better to do an actual check like if (svg) { .... than to do svg!.foo....

No changes needed here, test files it's fine to tell TS to be quiet, just wanted to point this out as a nugget for when you go on to (hopefully) use TS in client projects!

@sblinde
Copy link
Contributor Author

sblinde commented Aug 2, 2022

Looks good!

One thing I wanted to point out is the use of !, the non-null assertion operator. This TS operator basically says, "I'm telling you, the compiler, that I know this value isn't undefined or null". It's basically a way of telling TS to shut up a bit. For test files, which is where you have it used here, this isn't really a big deal. In application code, I always advise against this operator – as it waters down the usefulness of TS! It's generally better to do an actual check like if (svg) { .... than to do svg!.foo....

No changes needed here, test files it's fine to tell TS to be quiet, just wanted to point this out as a nugget for when you go on to (hopefully) use TS in client projects!

Thank you! Super helpful info!

@sblinde sblinde merged commit 0293d7e into main Aug 3, 2022
@sblinde sblinde deleted the feature/migrate-scatter-to-ts branch August 3, 2022 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: TypeScript Issues related to typescript or type definitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants