Skip to content

Migrate template to TypeScript#2143

Merged
raysonkoh merged 2 commits intoMarkBind:masterfrom
lhw-1:1913-typescript-template
Feb 13, 2023
Merged

Migrate template to TypeScript#2143
raysonkoh merged 2 commits intoMarkBind:masterfrom
lhw-1:1913-typescript-template

Conversation

@lhw-1
Copy link
Contributor

@lhw-1 lhw-1 commented Feb 7, 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.

Migrates packages/core/template/template.js to TypeScript following the workflow specified here.

This PR also builds on the changes made in PR #2148.

Overview of changes:

Anything you'd like to highlight/discuss:

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)


Checklist: ☑️

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

@lhw-1 lhw-1 changed the title Migrate template to TypeScript [WIP] Migrate template to TypeScript Feb 7, 2023
@lhw-1 lhw-1 marked this pull request as draft February 9, 2023 16:14
@lhw-1 lhw-1 force-pushed the 1913-typescript-template branch from 2a80042 to ad4dba8 Compare February 13, 2023 08:31
@lhw-1 lhw-1 closed this Feb 13, 2023
@lhw-1 lhw-1 force-pushed the 1913-typescript-template branch from ad4dba8 to 1dcef01 Compare February 13, 2023 08:34
@lhw-1 lhw-1 reopened this Feb 13, 2023
@lhw-1 lhw-1 marked this pull request as ready for review February 13, 2023 08:59
@lhw-1 lhw-1 changed the title [WIP] Migrate template to TypeScript Migrate template to TypeScript Feb 13, 2023
Copy link
Contributor

@jovyntls jovyntls left a comment

Choose a reason for hiding this comment

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

LGTM! TS migration is going well 🎊

@raysonkoh raysonkoh force-pushed the 1913-typescript-template branch from e4570fd to a20ef61 Compare February 13, 2023 15:24
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

@raysonkoh raysonkoh merged commit c32eb56 into MarkBind:master Feb 13, 2023
@tlylt
Copy link
Contributor

tlylt commented Feb 14, 2023

I think validateTemplateFromPath() { and generateSiteWithTemplate() { are suitable to be marked as private, making use of TypeScript to enforce access control. Should be ok for now since other migration PRs don't seem to deal with this aspect as well (or perhaps I overlooked it).

@tlylt tlylt added this to the 4.0.3 milestone Feb 14, 2023
@raysonkoh
Copy link
Contributor

Yea I think I general there needs to be some cleanup after the migration (which will hopefully be done soon) and marking suitable functions as private should be one aspect. The other migration PRs have not been focusing on this aspect.

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.

4 participants