Skip to content

fix: Implement fetch-retry locally #119

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

Merged
merged 2 commits into from
May 28, 2025
Merged

fix: Implement fetch-retry locally #119

merged 2 commits into from
May 28, 2025

Conversation

jonkafton
Copy link
Contributor

What are the relevant tickets?

Relates to https://github.com/mitodl/hq/issues/7238

Description (What does it do?)

Ports a subset of fetch-retry into the project.

How can this be tested?

Additional Context

#110 added fetch-retry as a dependency. Upgrading to the latest @mitodl/smoot-design in the MIT Learn project, the app runs without issue, though the Jest unit tests were seeing the error below:

  ● Channel Pages, Unit only › Displays the channel logo

    TypeError: (0 , fetch_retry_1.default) is not a function

      at Object.<anonymous> (../../../node_modules/@mitodl/smoot-design/dist/cjs/utils/retryingFetch.js:27:49)
      at Object.<anonymous> (../../../node_modules/@mitodl/smoot-design/dist/cjs/components/AiChat/AiChatContext.js:27:25)
      at Object.<anonymous> (../../../node_modules/@mitodl/smoot-design/dist/cjs/components/AiChat/AiChat.js:28:25)
      at Object.<anonymous> (../../../node_modules/@mitodl/smoot-design/dist/cjs/ai.js:4:16)
      at Object.<anonymous> (page-components/LearningResourceExpanded/AiChatSyllabusSlideDown.tsx:2336:27)
      at Object.<anonymous> (page-components/LearningResourceExpanded/LearningResourceExpanded.tsx:3306:66)
      at Object.<anonymous> (page-components/LearningResourceDrawer/LearningResourceDrawer.tsx:2861:27)
      at require (app-pages/ChannelPage/ChannelPage.tsx:15:5)

The tsc build complies ESM and CommonJS builds from smoot-design source, excluding any dependencies, which are imported directly in projects. The fetch-retry produces a single UMD build with Rollup. The app works (uses the UMD version which gets bundled by the app's bundler), though running Jest the tests fail when (using the CJS smoot-design version and the UMD fetch-retry module directly). Jest module system doesn't seem to handle UMD modules as well as modern bundlers.

Given the fetch-retry is small (full source), has no dependencies and we are only using a subset of it's API (we provide retryDelay and retryOn as functions), it makes best sense to find alternatives to fetch-retry that would work better with dual-format packages or just port the code into the project. This PR does the latter.

Copy link

Pre-release workflow has completed successfully.
Published version: 0.0.0-d04c42a

To update a previously-installed version with yarn:

yarn up @mitodl/[email protected]

@gumaerc gumaerc self-assigned this May 28, 2025
@jonkafton jonkafton added the Needs Review An open Pull Request that is ready for review label May 28, 2025
Copy link

@gumaerc gumaerc left a comment

Choose a reason for hiding this comment

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

This code works, I can see the requests retrying and eventually failing. Overall I think it makes more sense to implement these kinds of things locally anyway, because they're so simple. It feels like an easy target for a leftpad kind of attack.

Copy link

Pre-release workflow has completed successfully.
Published version: 0.0.0-bafccd3

To update a previously-installed version with yarn:

yarn up @mitodl/[email protected]

@jonkafton jonkafton removed the Needs Review An open Pull Request that is ready for review label May 28, 2025
@jonkafton jonkafton merged commit 6399805 into main May 28, 2025
9 checks passed
Copy link

🎉 This PR is included in version 6.6.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants