Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

feat(fetch): switch to cross-fetch instead of isomorphic-fetch #101

Merged
merged 3 commits into from
Apr 21, 2020

Conversation

JAdshead
Copy link
Contributor

@JAdshead JAdshead commented Apr 9, 2020

Description

Switching to cross-fetch instead of isomorphic-fetch as the later has not been maintained for 5 years.

Motivation and Context

isomorphic-fetch as the later has not been maintained for 5 years and provides outdated versions of both node-fetch and WHATWG fetch

How Has This Been Tested?

Unit and integration tests.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (adding or updating documentation)

Checklist:

  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • These changes should be applied to a maintenance branch.
  • This change requires cross browser checks.
  • Performance tests should be ran against the server prior to merging.
  • This change impacts caching for client browsers.
  • This change impacts HTTP headers.
  • This change adds additional environment variable requirements for One App users.
  • I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using One App?

Provides a fetch that is closer to the spec.

@JAdshead JAdshead requested review from a team as code owners April 9, 2020 00:24
@oneamexbot
Copy link
Contributor

oneamexbot commented Apr 9, 2020

📊 Bundle Size Report

file name size on disk gzip
app.js 112.6KB 31.4KB
runtime.js 15KB 5.3KB
vendors.js 128.1KB 37.9KB
app~vendors.js 405.9KB 106KB
legacy/app.js 119.4KB 33KB
legacy/runtime.js 15KB 5.3KB
legacy/vendors.js 163.2KB 44.9KB
legacy/app~vendors.js 412KB 107.6KB

Generated by 🚫 dangerJS against 668ac96

@infoxicator
Copy link
Contributor

@JAdshead Integration tests pass so looks like everything is working same as with isomorphic-fetch 😁, if I want to run some manual tests, is this the only dep I need? "@americanexpress/one-app-bundler": "^6.4.0",

@JAdshead
Copy link
Contributor Author

JAdshead commented Apr 14, 2020

@JAdshead Integration tests pass so looks like everything is working same as with isomorphic-fetch 😁, if I want to run some manual tests, is this the only dep I need? "@americanexpress/one-app-bundler": "^6.4.0",

I think you just need to be running this branch of the app. modules should not know about which fetch client is provided as its polyfilled

infoxicator
infoxicator previously approved these changes Apr 17, 2020
Copy link
Contributor

@infoxicator infoxicator left a comment

Choose a reason for hiding this comment

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

package-lock.json conflict needs fixing 👍

@mtomcal mtomcal requested review from a team April 21, 2020 18:27
@mtomcal
Copy link
Contributor

mtomcal commented Apr 21, 2020

This looks good and given how many Integration tests where we use fetch this gives me confidence this is a non breaking swapout.

@JAdshead JAdshead merged commit 82155d5 into master Apr 21, 2020
@JAdshead JAdshead deleted the feat/cross-fetch branch April 21, 2020 19:09
@JAdshead JAdshead added the enhancement New feature or request label Apr 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants