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

Migrate @woocommerce/date to TS #33108

Merged
merged 8 commits into from
May 24, 2022
Merged

Migrate @woocommerce/date to TS #33108

merged 8 commits into from
May 24, 2022

Conversation

chihsuan
Copy link
Member

@chihsuan chihsuan commented May 19, 2022 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

Closes #33100.

This PR migrates @woocommerce/number to TS and

  • fixes the wrong JSDoc types
  • adds missing deps moment-timezone to fix .tz() type undefined error

How to test the changes in this Pull Request:

  1. Run pnpm nx build @woocommerce/date
  2. Observe that packages/js/date/build-types` is generated
  3. Should run pnpm nx test @woocommerce/date without errors

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you created a changelog file for each project being changed, ie pnpm nx changelog <project>?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@github-actions github-actions bot added the package: @woocommerce/date issues related to @woocommerce/date label May 19, 2022
chihsuan
chihsuan commented May 19, 2022
View reviewed changes
packages/js/date/src/index.ts
export function getChartTypeForQuery( { chartType } ) {
if ( [ 'line', 'bar' ].includes( chartType ) ) {
export function getChartTypeForQuery( { chartType }: Query ) {
if ( chartType !== undefined && [ 'line', 'bar' ].includes( chartType ) ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

includes only accept string type.

Array<string>.includes(searchElement: string, fromIndex?: number | undefined): boolean

@botwoo
Copy link
Collaborator

botwoo commented May 19, 2022 •
edited
Loading

📊 Test reports for this pull request have been published and are accessible through the following links:

Latest commit referenced in the reports: Update tests fail messages 6c4674f
This comment will automatically be updated with the latest referenced commit when you push new changes to this pull request.

Visit the WooCommerce Test Reports homepage to view all published reports. See the FAQs page if you're having problems accessing them.

chihsuan
chihsuan commented May 19, 2022
View reviewed changes
packages/js/date/src/index.ts
* @param {string} [option.type] Date format type, d3 or php, defaults to d3.
* @return {string} Current interval.
*/
export function getDateFormatsForInterval(
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this function here to fix no-use-before-declare warning.

@chihsuan chihsuan marked this pull request as ready for review May 19, 2022 07:07
@chihsuan chihsuan self-assigned this May 19, 2022
@chihsuan chihsuan requested a review from a team May 19, 2022 07:07
ilyasfoo
ilyasfoo previously approved these changes May 23, 2022
View reviewed changes
Copy link
Contributor

@ilyasfoo ilyasfoo left a comment

Choose a reason for hiding this comment

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

This looks like quite an effort, thanks for wrangling this out, @chihsuan!

The change also was also significant, i.e: with the added null/undefined checks and error throws, so I've done a bit of testing in the analytics and dashboard to make sure none of the consumers were violating those checks. I found none, so I think we're good!

Just a nitpicky comment on test error message, but pre-approving!

packages/js/date/src/test/index.ts Outdated
@@ -71,19 +79,25 @@ describe( 'toMoment', () => {

it( 'should handle isoFormat dates', () => {
const myMoment = toMoment( 'YYYY', '2018-04-15' );
if ( myMoment === null ) fail( 'it should not reach here' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick but I'd prefer the fail message to be more precise, i.e: "myMoment should not be null" or something like that

Copy link
Member Author

Choose a reason for hiding this comment

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

@ilyasfoo Thanks for the review. 🙌 Changed them in 6c4674f

@ilyasfoo ilyasfoo self-requested a review May 24, 2022 06:20
ilyasfoo
ilyasfoo approved these changes May 24, 2022
View reviewed changes
Copy link
Contributor

@ilyasfoo ilyasfoo left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM 🚢

@chihsuan chihsuan merged commit c5ccaa0 into trunk May 24, 2022
@chihsuan chihsuan deleted the dev/33100-migrate-woo-date-to-ts branch May 24, 2022 06:21
@github-actions github-actions bot mentioned this pull request Jul 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilyasfoo ilyasfoo ilyasfoo approved these changes

Assignees

@chihsuan chihsuan

Labels
package: @woocommerce/date issues related to @woocommerce/date
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Migrate @woocommerce/date to TS
3 participants
@chihsuan @botwoo @ilyasfoo