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

[V3] Remove cross-fetch from template-retail-react-app #1160

Merged
merged 3 commits into from
Apr 28, 2023
Merged

Conversation

bendvc
Copy link
Collaborator

@bendvc bendvc commented Apr 28, 2023

Description

We recently removed the commerce-api client code from the template, leaving behind the einstein implementation. Because einstein is only ever used on the client we don't need to use cross-fetch, so we are removing it in this PR.

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Changes

  • Remove cross-fetch from useEinstein hook
  • Fix tests to mock global fetch.
  • Remove dep in package.json and re-lock

How to Test-Drive This PR

  • Run tests, they should pass.
  • Preview the template and navigate from page to page, you should see fetch logs for viewCategory viewProduct etc. This means things are still working.

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

@bendvc bendvc requested a review from a team as a code owner April 28, 2023 21:43
kevinxh
kevinxh previously approved these changes Apr 28, 2023
beforeEach(() => {
jest.resetModules()
fetch.mockClear()
const fetchMock = require('jest-fetch-mock')
Copy link
Collaborator

@alexvuong alexvuong Apr 28, 2023

Choose a reason for hiding this comment

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

Are we installing this package? Do we need this package? I tried this approach, and it does work without importing this library

let originalFetch

    beforeEach(() => {
        originalFetch = global.fetch
        global.fetch = jest.fn(() =>
            Promise.resolve({
                json: jest.fn(),
                ok: true
            })
        )
    })

    afterEach(() => {
        global.fetch = originalFetch
    })

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The lib was already installed and in dev dependancies. But I'm not sure what it was used for before. I think it was used before MSW as added maybe?

Anyway, it doesn't hurt to use a mock fetch lib that was already installed, rather than a one off mock. It might give more flexibility in the future if we have tests that do more than mock a successful call.

@bendvc bendvc merged commit 4ff5e72 into v3 Apr 28, 2023
bfeister added a commit that referenced this pull request May 9, 2023
* v3:
  fix: remove device-context and detect-device-type (#1168)
  Dependency updates (#1170)
  [Snyk] Security upgrade cosmiconfig from 7.1.0 to 8.0.0 (#1145)
  [V3] Remove `cross-fetch` from `template-retail-react-app` (#1160)
  [v3] Add suffix to ssr build files (#1158)
  Consume new version of the commerce-sdk-isomorphic to enable Phased Launches support HTTP Basic Auth (#1153)

# Conflicts:
#	packages/commerce-sdk-react/package-lock.json
#	packages/internal-lib-build/package-lock.json
#	packages/pwa-kit-runtime/package-lock.json
#	packages/template-retail-react-app/package-lock.json
bfeister added a commit that referenced this pull request May 9, 2023
…xtensibility-algo-refactor + regen lockfiles

* feature/template-extensibility:
  regen package lockfiles
  fix: remove device-context and detect-device-type (#1168)
  Dependency updates (#1170)
  [Snyk] Security upgrade cosmiconfig from 7.1.0 to 8.0.0 (#1145)
  [V3] Remove `cross-fetch` from `template-retail-react-app` (#1160)
  [v3] Add suffix to ssr build files (#1158)
  Consume new version of the commerce-sdk-isomorphic to enable Phased Launches support HTTP Basic Auth (#1153)

# Conflicts:
#	package-lock.json
#	packages/commerce-sdk-react/package-lock.json
#	packages/internal-lib-build/package-lock.json
#	packages/my-extended-retail-app/package-lock.json
#	packages/pwa-kit-create-app/package-lock.json
#	packages/pwa-kit-dev/package-lock.json
#	packages/pwa-kit-react-sdk/package-lock.json
#	packages/pwa-kit-runtime/package-lock.json
#	packages/template-retail-react-app/app/hooks/use-einstein.test.js
#	packages/template-retail-react-app/package-lock.json
#	packages/template-typescript-minimal/package-lock.json
#	packages/test-commerce-sdk-react/package-lock.json
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.

3 participants