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

refactor: migrate typescript #5092

Merged
merged 45 commits into from
Mar 15, 2023
Merged

refactor: migrate typescript #5092

merged 45 commits into from
Mar 15, 2023

Conversation

stevenjoezhang
Copy link
Member

@stevenjoezhang stevenjoezhang commented Nov 1, 2022

What does it do?

Issue resolved: #4136

Screenshots

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.

@github-actions
Copy link

github-actions bot commented Nov 1, 2022

.husky/pre-commit Outdated Show resolved Hide resolved
@yoshinorin
Copy link
Member

@stevenjoezhang
BTW, do you have any plans to merge current open PRs before merging this PR? As we know, other PRs will be difficult to merge if we merge this. I assume you re-opened other PRs for it problem.

@stevenjoezhang
Copy link
Member Author

Yes, we need to do a lot of rebase before / after this PR merged

@renbaoshuo
Copy link
Member

It seems some conflicts appeared after merging #5136.

@yoshinorin
Copy link
Member

yoshinorin commented Dec 31, 2022

It seems some conflicts appeared after merging #5136.

Yes. We have to resolve conflicts every time after merging other PRs. Personally, I want to merge this PR prioritize over others and release v7.0.0. Of course, if we can merge (or reject) other PRs soon that is better. But I think it is difficult for us.

@renbaoshuo
Copy link
Member

@hexojs/core

I think we should merge #4926 and #4560 before merging this PR so that we can include all of them in next major release.

In addition, we should close #3719 and apply prettier after merging this PR.

@yoshinorin
Copy link
Member

@stevenjoezhang
I added some commits for the success build (not yet completed). Today I read the current branch's source code. I'm not familiar with TS but I assume we have to spend many rewrites & much time to migrate TS. So, I think we should use @ts-ignore for migrate to TS once. After merge this PR we remove @ts-ignore step by step.

How do you think?
Thanks :)

@SukkaW
Copy link
Member

SukkaW commented Feb 15, 2023

I added some commits for the success build (not yet completed). Today I read the current branch's source code. I'm not familiar with TS but I assume we have to spend many rewrites & much time to migrate TS. So, I think we should use @ts-ignore for migrate to TS once. After merge this PR we remove @ts-ignore step by step.

IMHO we should prefer @ts-expect-error and as any (instead of @ts-ignore) whenever possible. But yes, we can always bring up type later.

@coveralls
Copy link

coveralls commented Mar 7, 2023

Coverage Status

Coverage: 98.635% (-0.05%) from 98.687% when pulling 2749b23 on ts into 7edbf25 on master.

@stevenjoezhang
Copy link
Member Author

It can be merged after fixing the eslint error

@stevenjoezhang stevenjoezhang marked this pull request as ready for review March 7, 2023 16:11
yoshinorin
yoshinorin previously approved these changes Mar 8, 2023
Copy link
Member

@yoshinorin yoshinorin left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ❤️ 🎉

It works correctly on my local machine. But, IMHO we should merge #5145 before merge this. Also #5153 and #4926? But, I don't know how to deal with #4926 🤔

@stevenjoezhang
Copy link
Member Author

And we can release a candidate version like 7.0.0-rc.1 first, in case there are still some typescript issues that are not fully fixed

@renbaoshuo
Copy link
Member

I noticed there are still some lint errors...

@yoshinorin
Copy link
Member

yoshinorin commented Mar 9, 2023

And we can release a candidate version like 7.0.0-rc.1 first, in case there are still some typescript issues that are not fully fixed

So, our release plan is...

  1. create v7.0.0 branch
  2. merge ts branch into v7.0.0 branch
  3. create a PR to v7.0.0 branch for release v7.0.0-rc1
  4. release v7.0.0-rc1 from v7.0.0 branch
  5. review perf(post): cache tags getter #5145, fix(post-asset): strip extensions better on permalink #5153 and Async rendering of tags within a post #4926
    a. We will merge them into master branch or reject
  6. merge v7.0.0 branch into master and resolve conflict if needed
    a. if v7.0.0-rcN hasn't critical issues
  7. release v7.0.0

Correct? And may we proceed this plan?

@renbaoshuo
Copy link
Member

image

Hmm... It seems that there are anything wrong with our eslint rules?

@stevenjoezhang
Copy link
Member Author

Seems that the eslint rules of js and ts are in conflict...

@yoshinorin yoshinorin changed the base branch from master to v7.0.0 March 14, 2023 10:20
Copy link
Member

@yoshinorin yoshinorin left a comment

Choose a reason for hiding this comment

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

#5092 (comment)

I pushed the v7.0.0 branch and changed the base branch from master to v7.0.0. We can merge this PR into v7.0.0 at any time.

I'm going to submit PR for v7.0.0-rc1 after merge this.

Thank you :)

@yoshinorin yoshinorin merged commit 148ed7c into v7.0.0 Mar 15, 2023
@yoshinorin
Copy link
Member

refs: #5176

@stevenjoezhang stevenjoezhang deleted the ts branch March 15, 2023 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to TypeScript
5 participants