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: added missing push and reminders related types #942

Merged
merged 3 commits into from
Apr 5, 2022
Merged

fix: added missing push and reminders related types #942

merged 3 commits into from
Apr 5, 2022

Conversation

vishalnarkhede
Copy link
Contributor

@vishalnarkhede vishalnarkhede commented Apr 5, 2022 •
edited
Loading

Changelog

  • Fixed the typescript for push config
  • Added missing types for automod_threshold
  • Added missing types for reminders
  • Move the api keys and secrets for typescript test to github secret

ferhatelmas
ferhatelmas previously approved these changes Apr 5, 2022
View reviewed changes
@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2022 •
edited
Loading

Size Change: 0 B

Total Size: 298 kB

ℹ️ View Unchanged
Filename Size Change
dist/browser.es.js 65.3 kB 0 B
dist/browser.full-bundle.min.js 35.7 kB 0 B
dist/browser.js 66 kB 0 B
dist/index.es.js 65.4 kB 0 B
dist/index.js 66.1 kB 0 B

compressed-size-action

@vishalnarkhede vishalnarkhede force-pushed the vishal/ts-issues branch 5 times, most recently from a9d5194 to 98659c2 Compare April 5, 2022 15:25
peterdeme
peterdeme reviewed Apr 5, 2022
View reviewed changes
src/types.ts
Comment on lines -1959 to +1965
roles?: string[];
roles?: string[] | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't ? means it's nullable already?

Copy link
Contributor

Choose a reason for hiding this comment

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

? means string[] | undefined which is a bit different from null. So the result type here is string[] undefined | null

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah but does it make sense here?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah but does it make sense here?

That's a good question :) I don't think this should be necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not to me ... but backend sent null in one of our test case

Copy link
Contributor Author

@vishalnarkhede vishalnarkhede Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I can remove it if we can ensure its never null from backend response

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep null since this was sent from the backend

.github/workflows/type.yml Outdated Show resolved Hide resolved
peterdeme
peterdeme reviewed Apr 5, 2022
View reviewed changes
src/types.ts Show resolved Hide resolved
peterdeme
peterdeme reviewed Apr 5, 2022
View reviewed changes
src/types.ts Show resolved Hide resolved
peterdeme
peterdeme reviewed Apr 5, 2022
View reviewed changes
src/types.ts Show resolved Hide resolved
@vishalnarkhede vishalnarkhede marked this pull request as draft April 5, 2022 15:35
@vishalnarkhede vishalnarkhede mentioned this pull request Apr 5, 2022
Merged
2 tasks
@vishalnarkhede vishalnarkhede changed the title fix: added missing push and reminders related types [WIP] fix: added missing push and reminders related types Apr 5, 2022
@vishalnarkhede vishalnarkhede force-pushed the vishal/ts-issues branch 7 times, most recently from dea0547 to cd44660 Compare April 5, 2022 17:02
@vishalnarkhede vishalnarkhede changed the title [WIP] fix: added missing push and reminders related types fix: added missing push and reminders related types Apr 5, 2022
@vishalnarkhede vishalnarkhede marked this pull request as ready for review April 5, 2022 17:08
@vishalnarkhede vishalnarkhede enabled auto-merge (squash) April 5, 2022 19:21
@vishalnarkhede vishalnarkhede merged commit b7543eb into master Apr 5, 2022
@vishalnarkhede vishalnarkhede deleted the vishal/ts-issues branch April 5, 2022 19:21
@github-actions github-actions bot mentioned this pull request Apr 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterdeme peterdeme peterdeme left review comments

@AnatolyRugalev AnatolyRugalev AnatolyRugalev approved these changes

@ferhatelmas ferhatelmas ferhatelmas left review comments

@gumuz gumuz Awaiting requested review from gumuz

@madsroskar madsroskar Awaiting requested review from madsroskar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vishalnarkhede @ferhatelmas @AnatolyRugalev @peterdeme