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

Rewrite actions/modal and reducers/modal with typescript #24833

Merged
merged 27 commits into from May 25, 2023
Merged

Rewrite actions/modal and reducers/modal with typescript #24833

merged 27 commits into from May 25, 2023

Conversation

takayamaki
Copy link
Contributor

This PR rewrites actions/modal and reducers/modal with typescript and createAction.

nschonni
nschonni reviewed May 4, 2023
View reviewed changes
app/javascript/mastodon/reducers/modal.ts Outdated
case TIMELINE_DELETE:
return state.update('stack', (stack) =>
stack.filterNot(
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the extra ignore is required if you put an explanation string after the @ts-expect-error. The error should probably be addressed though

@renchap renchap self-requested a review May 4, 2023 10:35
@renchap
Copy link
Sponsor Member

renchap commented May 4, 2023

Thanks for continuing working on this <3

I will have a look, but I would like to merge #23631 before any other TS PRs are merged.

@takayamaki takayamaki force-pushed the refactor/rewrite_modal_action_with_createAction branch 2 times, most recently from 146cbfd to 54a82a9 Compare May 9, 2023 19:38
renchap
renchap requested changes May 9, 2023
View reviewed changes
Copy link
Sponsor Member

@renchap renchap left a comment

Choose a reason for hiding this comment

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

Nice work on this, just a small nitpick (and I think you need to rebase it, and re-run eslint with my recent changes?)

app/javascript/mastodon/reducers/modal.ts Outdated
});
};

export function modal(state = initialState, action: PayloadAction<any>) {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Can you rename it modalReducer so its more explicit?

@renchap
Copy link
Sponsor Member

renchap commented May 9, 2023

Also, could it be a good place to start rewriting the reducer with createReducer (or createSlice) from RTK?

This specific modal state seem to only be used in modal_container.js, so it can be easily converted into an Immer reducer.

If you feel confident with this, you can open another PR after this one doing it, or I can have a go as well. I would like a simple example of how a full rewrite to Typescript look like (Actions, Reducer, removing Immer, then rewriting into a FunctionComponent using Redux's hooks)

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@takayamaki
Copy link
Contributor Author

Also, could it be a good place to start rewriting the reducer with createReducer (or createSlice) from RTK?

This specific modal state seem to only be used in modal_container.js, so it can be easily converted into an Immer reducer.

If you feel confident with this, you can open another PR after this one doing it, or I can have a go as well. I would like a simple example of how a full rewrite to Typescript look like (Actions, Reducer, removing Immer, then rewriting into a FunctionComponent using Redux's hooks)

OK, I will try that next week.
I attend RubyKaigi this week.

@takayamaki takayamaki force-pushed the refactor/rewrite_modal_action_with_createAction branch from b905c95 to cf44df1 Compare May 18, 2023 15:38
@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@takayamaki takayamaki force-pushed the refactor/rewrite_modal_action_with_createAction branch from cf44df1 to 911f569 Compare May 18, 2023 15:53
@takayamaki
Copy link
Contributor Author

takayamaki commented May 18, 2023 •
edited

rebased.

I tried rewriting this reducer with createReducer.
However, it is need to rewrite changeUploadCompose by createAsyncThunk without ts support.

It is difficult for me, and I seem that is too big to include this PR.

renchap
renchap requested changes May 19, 2023
View reviewed changes
Copy link
Sponsor Member

@renchap renchap 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 to me, except the modalReducer renaming in the state, it should be kept as key.

I missed that this was using async thunks, I will have a look later on the best way to handle this.

I think we could also have OpenModalPayload as a generic type, which would allow to properly type modalProps, I will try to find a way to make it work once this PR is merged.

Thanks again for this, good work!

app/javascript/mastodon/reducers/index.ts Outdated
@@ -50,7 +50,7 @@ const reducers = {
meta,
alerts,
loadingBar: loadingBarReducer,
modal,
modalReducer,
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

You should not change the key here, but use modal: modalReducer.

This would avoid changing the modal key everywhere this is accessed

@takayamaki takayamaki force-pushed the refactor/rewrite_modal_action_with_createAction branch from 911f569 to a8f9523 Compare May 19, 2023 10:50
@takayamaki takayamaki requested a review from renchap May 19, 2023 10:50
@ClearlyClaire ClearlyClaire requested review from ClearlyClaire and removed request for nschonni May 19, 2023 15:06
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@takayamaki takayamaki force-pushed the refactor/rewrite_modal_action_with_createAction branch from 1d45331 to def9a79 Compare May 23, 2023 18:05
@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@takayamaki
rewrite AUDIO modal caller
33dd22f
@takayamaki
rewrite IMAGE modal caller
1383e6e
@takayamaki
rewrite BOOST modal caller
43593c4
@takayamaki
rewrite CONFIRM modal caller
cdf0184
@takayamaki
rewrite MUTE modal caller
90d6990
@takayamaki
rewrite BLOCK modal caller
db8ef3c
@takayamaki
rewrite REPORT modal caller
6a234ab
@takayamaki
rewrite ACTIONS modal caller
963c75d
@takayamaki
rewrite EMBED modal caller
276e3c4
@takayamaki
rewrite LIST_EDITOR modal caller
6f57f22
@takayamaki
rewrite FOCAL_POINT modal caller
a0d76fb
@takayamaki
rewrite LIST_ADDER modal caller
1b3427a
@takayamaki
rewrite COMPARE_HISTORY modal caller
5412ca0
@takayamaki
rewrite FILTER modal caller
06f7af7
@takayamaki
rewrite SUBSCRIBED_LANGUAGES modal caller
0985284
@takayamaki
rewrite INTERACTION modal caller
73bcfab
@takayamaki
rewrite CLOSED_REGISTRATIONS modal caller
7b94c09
@takayamaki
rewrite closeModal caller
f13d216
@takayamaki
format actions/modal and reducers/modal
f29fb38
@takayamaki
Use named export
b3ed5c3
@takayamaki
rename to modalReducer
9a971e0
@takayamaki
Embed type annotations into arrow function definitions
8fc0b2e
@takayamaki takayamaki force-pushed the refactor/rewrite_modal_action_with_createAction branch from def9a79 to 8fc0b2e Compare May 25, 2023 12:42
@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@takayamaki
Copy link
Contributor Author

Let's merge this, can you rebase one last time? 🙏

rebased!

@renchap renchap merged commit 38c6216 into mastodon:main May 25, 2023
25 checks passed
@takayamaki takayamaki deleted the refactor/rewrite_modal_action_with_createAction branch May 25, 2023 13:58
skerit pushed a commit to 11ways/mastodon that referenced this pull request Jul 7, 2023
@takayamaki @skerit
Rewrite actions/modal and reducers/modal with typescript (mastodon#24833
f67b5cb 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ClearlyClaire ClearlyClaire ClearlyClaire left review comments

@nschonni nschonni nschonni left review comments

@renchap renchap renchap approved these changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@takayamaki @renchap @ClearlyClaire @nschonni