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

Convert some common classes/utils to TS #2929

Merged
merged 8 commits into from
Nov 23, 2021

Conversation

dsevillamartin
Copy link
Member

@dsevillamartin dsevillamartin commented Jun 20, 2021

Changes proposed in this pull request:

  • Convert Application, Session, utils/extractText
  • Update states/AlertManagerState and utils/RequestError

Reviewers should focus on:

  • Properly typing extract is close to impossible because we override it with a different type. We use it and just pass xhr.responseText, but Mithril's extract option takes xhr and options.

    PhpStorm screenshots

    image
    image

  • Do we want the Application types to be exported at the top or a different file?

Screenshot

Confirmed

  • Frontend changes: tested on a local Flarum installation.

@SychO9 SychO9 added this to the 1.1 milestone Jun 27, 2021
@davwheat davwheat force-pushed the ds/convert-main-classes-to-ts branch from f6dd14f to d29d4d6 Compare August 18, 2021 23:07
@davwheat
Copy link
Member

Rebased onto master. I've dropped Application's conversion in favour of more recent progress on #3006. I didn't realise you did it in here, but the new PR is more likely to be up-to-date with new changes, and has support for new ItemList typings from #3005.

js/src/common/Session.ts Outdated Show resolved Hide resolved
js/src/common/Session.ts Outdated Show resolved Hide resolved
js/src/common/Session.ts Outdated Show resolved Hide resolved
js/src/common/utils/extractText.ts Outdated Show resolved Hide resolved
js/src/common/Session.ts Outdated Show resolved Hide resolved
js/src/common/Session.ts Outdated Show resolved Hide resolved
js/src/common/Session.ts Show resolved Hide resolved
js/src/common/states/AlertManagerState.ts Show resolved Hide resolved
js/src/common/utils/extractText.ts Outdated Show resolved Hide resolved
js/src/common/utils/extractText.ts Outdated Show resolved Hide resolved
@askvortsov1 askvortsov1 modified the milestones: 1.1, 1.2 Sep 28, 2021
js/src/common/Session.ts Outdated Show resolved Hide resolved
js/src/common/states/AlertManagerState.ts Show resolved Hide resolved
js/src/common/utils/RequestError.ts Outdated Show resolved Hide resolved
js/src/common/utils/extractText.ts Outdated Show resolved Hide resolved
@askvortsov1
Copy link
Member

Before review, this should be rebased, and tested against the current TypeScript environment to ensure that it doesn't introduce any new bugs.

@dsevillamartin
Copy link
Member Author

@askvortsov1 I rebased this PR a week ago, that's why I re-requested the reviews.

@askvortsov1
Copy link
Member

@askvortsov1 I rebased this PR a week ago, that's why I re-requested the reviews.

Right, but the Typecheck and type coverage GH actions aren't showing up, so those must have been merged after you rebased.

Copy link
Member

@davwheat davwheat 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!

Not sure if these suggestions might be better as Babel compiles them to a smaller bundle size. Up to you.

js/src/common/utils/extractText.ts Show resolved Hide resolved
js/src/common/utils/extractText.ts Show resolved Hide resolved
@dsevillamartin
Copy link
Member Author

@davwheat I'd rather keep it as is, I feel like String() is clearer.

@dsevillamartin dsevillamartin merged commit aa0b68b into master Nov 23, 2021
@dsevillamartin dsevillamartin deleted the ds/convert-main-classes-to-ts branch November 23, 2021 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants