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 application files to TS #3006

Merged
merged 14 commits into from
Oct 30, 2021
Merged

Rewrite application files to TS #3006

merged 14 commits into from
Oct 30, 2021

Conversation

davwheat
Copy link
Member

@davwheat davwheat commented Aug 9, 2021 •
edited
Loading

Changes proposed in this pull request:

  • Rewrites Application JS files to TS
  • Rewrites some closely related files that affected the typing abilities of Application

Reviewers should focus on:

Any more specific/accurate types we could use?

Confirmed

  • Frontend changes: tested on a local Flarum installation.

@davwheat davwheat added this to the 1.0.5 milestone Aug 9, 2021
@davwheat davwheat self-assigned this Aug 9, 2021
SychO9
SychO9 previously approved these changes Aug 9, 2021
View reviewed changes
@dsevillamartin
Copy link
Member

dsevillamartin commented Aug 9, 2021 •
edited
Loading

There's a resolver optional key apart from component & path, I think?

Yes - https://github.com/flarum/core/blob/eb4b18a979c7406cbf154a107662652d282fe415/js/src/common/utils/mapRoutes.js#L19

Either component or resolver are needed, not both.
If component is passed, you can also have resolverClass.

https://github.com/flarum/core/blob/eb4b18a979c7406cbf154a107662652d282fe415/js/src/common/utils/mapRoutes.js#L22

@askvortsov1
Copy link
Member

There's a resolver optional key apart from component & path, I think?

Yes - https://github.com/flarum/core/blob/eb4b18a979c7406cbf154a107662652d282fe415/js/src/common/utils/mapRoutes.js#L19

Either component or resolver are needed, not both.
If component is passed, you can also have resolverClass.

https://github.com/flarum/core/blob/eb4b18a979c7406cbf154a107662652d282fe415/js/src/common/utils/mapRoutes.js#L22

Let's enforce this with a union type! Yay for type safety!

askvortsov1
askvortsov1 reviewed Aug 14, 2021
View reviewed changes
Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Why not just convert Application to TS? That could be one of the most valuable things to type.

@davwheat
Copy link
Member Author

I planned for this to just be an intermediary step, but screw it.

TYPESCRIPT, HERE WE COME! 🚀

@SychO9 SychO9 modified the milestones: 1.0.5, 1.1 Aug 15, 2021
@davwheat davwheat force-pushed the dw/fix-app-typings branch 2 times, most recently from 4a4242e to 382e340 Compare August 17, 2021 19:32
@davwheat davwheat changed the title Replace Object typings with Record<string, unknown> Rewrite application files to TS Aug 18, 2021
@davwheat davwheat mentioned this pull request Aug 18, 2021
Merged
1 task
askvortsov1
askvortsov1 requested changes Aug 19, 2021
View reviewed changes
js/src/common/Application.tsx
}
);

export interface RouteResolver<
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we use Mithril's RouteResolver?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is more specific to our use case, in terms of typing attributes and the fact that Flarum uses its Component class for basically every component.

@davwheat
Copy link
Member Author

Rebased onto master to fix the conflicts

davwheat and others added 4 commits September 10, 2021 20:44
@davwheat @askvortsov1
Address some review comments
0769a63 
Co-authored-by: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com>
@davwheat
Fix build error
740d765
@davwheat @askvortsov1
Address some review comments
7b0b785 
Co-authored-by: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com>
@davwheat
Add type import qualifier
297ce78
@SychO9 SychO9 dismissed their stale review September 11, 2021 08:26

Stale review

@askvortsov1 askvortsov1 requested a review from SychO9 September 13, 2021 19:31
@tankerkiller125 tankerkiller125 removed their request for review September 19, 2021 02:10
@askvortsov1 askvortsov1 modified the milestones: 1.1, 1.2 Sep 28, 2021
@davwheat
Copy link
Member Author

LET'S GET TYPESCRIPTIN'

@davwheat davwheat merged commit f8232b9 into master Oct 30, 2021
@davwheat davwheat deleted the dw/fix-app-typings branch October 30, 2021 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SychO9 SychO9 SychO9 approved these changes

@askvortsov1 askvortsov1 askvortsov1 approved these changes

@dsevillamartin dsevillamartin Awaiting requested review from dsevillamartin

Assignees

@davwheat davwheat

Labels
type/cleanup
Projects
None yet
Milestone
1.2
Development

Successfully merging this pull request may close these issues.
4 participants
@davwheat @dsevillamartin @askvortsov1 @SychO9