Skip to content

Conversation

@moon0326
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

Closes #32881 .

This PR updates history package to the latest 5.3.0 It also removes @types/history as it is part of hisotry now.

Please keep in mind that we're updating to a major version with the following breaking changes. We're not affected from the braking changes as far as I know.

  • Removed support for browsers that do not support the HTML5 history API (no pushState)
  • Removed relative pathname support in hash history and memory history
  • Removed getUserConfirmation, keyLength, and hashType APIs

How to test the changes in this Pull Request:

There are no visual changes.

  • Please visit as many pages as possible and make sure no errors are shown.
  • Please also enable the navigation from WooCommerce -> Settings -> Advanced -> Features and make sure the Navigation works as expected without any 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.

@moon0326 moon0326 requested a review from a team May 13, 2022 01:37
@github-actions github-actions bot added focus: react admin package: @woocommerce/navigation issues related to @woocommerce/navigation plugin: woocommerce Issues related to the WooCommerce Core plugin. labels May 13, 2022
Copy link
Member

@chihsuan chihsuan left a comment

Choose a reason for hiding this comment

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

@moon0326 Looks good to me. But it seems that one unit test is failing due to the package update.

Summary of all failing tests
 FAIL  navigation/components/container/test/index.js
  ● Container › should update the active item and level when location changes

    TypeError: window.location.assign is not a function

      40 |             },
      41 |             createHref: (...args) => browserHistory.createHref.apply(browserHistory, args),
    > 42 |             push: (...args) => browserHistory.push.apply(browserHistory, args),
         |                                                    ^
      43 |             replace: (...args) => browserHistory.replace.apply(browserHistory, args),
      44 |             go: (...args) => browserHistory.go.apply(browserHistory, args),
      45 |             goBack: (...args) => browserHistory.goBack.apply(browserHistory, args),

      at Object.push (../../../node_modules/.pnpm/[email protected]/node_modules/history/umd/history.development.js:240:27)
      at Object.push (../../../packages/js/navigation/build/history.js:42:52)
      at navigation/components/container/test/index.js:157:17
      at batchedUpdates$1 (../../../node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom.development.js:22380:12)
      at act (../../../node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom-test-utils.development.js:1042:14)
      at Object.<anonymous> (navigation/components/container/test/index.js:152:12)


Test Suites: 1 failed, 85 passed, 86 total
Tests:       1 failed, 577 passed, 578 total
Snapshots:   20 passed, 20 total
Time:        31.784 s

@moon0326 moon0326 force-pushed the update/update-history-package branch from 9d21a53 to 29fcbfa Compare May 19, 2022 22:02
@botwoo
Copy link
Collaborator

botwoo commented May 19, 2022

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

Latest commit referenced in the reports: Fix the failing test 29fcbfa
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.

@moon0326
Copy link
Contributor Author

@moon0326 Looks good to me. But it seems that one unit test is failing due to the package update.

Summary of all failing tests
 FAIL  navigation/components/container/test/index.js
  ● Container › should update the active item and level when location changes

    TypeError: window.location.assign is not a function

      40 |             },
      41 |             createHref: (...args) => browserHistory.createHref.apply(browserHistory, args),
    > 42 |             push: (...args) => browserHistory.push.apply(browserHistory, args),
         |                                                    ^
      43 |             replace: (...args) => browserHistory.replace.apply(browserHistory, args),
      44 |             go: (...args) => browserHistory.go.apply(browserHistory, args),
      45 |             goBack: (...args) => browserHistory.goBack.apply(browserHistory, args),

      at Object.push (../../../node_modules/.pnpm/[email protected]/node_modules/history/umd/history.development.js:240:27)
      at Object.push (../../../packages/js/navigation/build/history.js:42:52)
      at navigation/components/container/test/index.js:157:17
      at batchedUpdates$1 (../../../node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom.development.js:22380:12)
      at act (../../../node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom-test-utils.development.js:1042:14)
      at Object.<anonymous> (navigation/components/container/test/index.js:152:12)


Test Suites: 1 failed, 85 passed, 86 total
Tests:       1 failed, 577 passed, 578 total
Snapshots:   20 passed, 20 total
Time:        31.784 s

Thank you for catching it! Fixed in 29fcbfa

Copy link
Member

@chihsuan chihsuan left a comment

Choose a reason for hiding this comment

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

LGTM🚀

@moon0326 moon0326 merged commit 0b35ad6 into trunk May 20, 2022
@moon0326 moon0326 deleted the update/update-history-package branch May 20, 2022 01:27
@github-actions github-actions bot added this to the 6.7.0 milestone May 20, 2022
@github-actions
Copy link
Contributor

Hi @moon0326, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the release: add testing instructions label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: @woocommerce/navigation issues related to @woocommerce/navigation plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update history-related JS dependencies

4 participants