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

Migrate Site/index.js to TypeScript #2270

Merged
merged 2 commits into from
Apr 15, 2023

Conversation

jovyntls
Copy link
Contributor

@jovyntls jovyntls commented Apr 8, 2023

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Part of #1913

Overview of changes:
Migrates Site/index.js to TS

Anything you'd like to highlight/discuss:
If we want to refactor how the methods at the end of Site/index.js are handled, I'd suggest we do a separate refactor of these methods after migrating this file to TS. Rationale of having these methods defined with Site.prototype: see #979

Testing instructions:
N/A

Proposed commit message: (wrap lines at 72 characters)
(rebase commit)


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@jovyntls jovyntls force-pushed the ts-migrate-site-indexjs branch 2 times, most recently from 478bbd5 to 0f12a21 Compare April 10, 2023 06:54
@jovyntls jovyntls requested a review from a team April 10, 2023 10:36
Copy link
Contributor

@raysonkoh raysonkoh left a comment

Choose a reason for hiding this comment

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

Thanks for the hardwork! Added some preliminary comments.

packages/core/src/Site/index.ts Show resolved Hide resolved
packages/core/src/Site/index.ts Outdated Show resolved Hide resolved
packages/core/src/Site/index.ts Show resolved Hide resolved
packages/core/src/Site/index.ts Outdated Show resolved Hide resolved
packages/core/src/Site/index.ts Outdated Show resolved Hide resolved
packages/core/src/Site/index.ts Show resolved Hide resolved
packages/core/src/Site/index.ts Outdated Show resolved Hide resolved
packages/core/src/Site/index.ts Show resolved Hide resolved
@jovyntls jovyntls force-pushed the ts-migrate-site-indexjs branch 2 times, most recently from 9584d6b to c598c07 Compare April 13, 2023 01:52
Copy link
Contributor

@raysonkoh raysonkoh left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @jovyntls!

Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

Thanks @jovyntls for this 💪, have taken a brief look and LGTM.

@tlylt tlylt added this to the v4.1.1 milestone Apr 14, 2023
@jovyntls jovyntls merged commit 63ed852 into MarkBind:master Apr 15, 2023
4 checks passed
@jovyntls jovyntls deleted the ts-migrate-site-indexjs branch April 15, 2023 10:25
@ang-zeyu
Copy link
Contributor

@tlylt as it will be quite a bit of work to undo the revert while keeping track of new PRs editing the file #2308 (comment), I suggest adding a small follow up comment on top that prohibits any changes to this file until this is fixed (and bumping the priority).

The alternative is letting a portion of the typescript migration work go to waste (also an ok approach).
I'll leave this up to you.

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.

None yet

4 participants