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

Upgrade cross-fetch and remove node-fetch #608

Merged
merged 5 commits into from Jan 25, 2022
Merged

Upgrade cross-fetch and remove node-fetch #608

merged 5 commits into from Jan 25, 2022

Conversation

minkimcello
Copy link
Collaborator

@minkimcello minkimcello commented Jan 24, 2022

Motivation

To resolve #607

Approach

  • Upgrade cross-fetch to 3.1.5
  • Remove node-fetch
  • Update @effection/process tests
  • Added change file for a patch bump of @effection/fetch only as the changes for @effection/process in dev env only

@minkimcello minkimcello changed the title WIP Upgrade cross-fetch and remove node-fetch Upgrade cross-fetch and remove node-fetch Jan 24, 2022
@minkimcello minkimcello marked this pull request as ready for review January 24, 2022 15:07
Comment on lines 47 to 51
yield expect(fetch(`http://localhost:29000`, { method: "POST", body: "hello" })).rejects.toHaveProperty('name', 'FetchError');
let error;
try {
yield fetch(`http://localhost:29000`, { method: "POST", body: "hello" })
} catch (err) {
error = err;
}
expect(error).toHaveProperty('name', 'FetchError');
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this wasn't working the same as before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It returns the Fetch interface and not the response without the yield

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see we're using the Effection fetch here. If we're going to treat the error as a value, then let's just capture it as a return value instead of re-assigning a variable.

let error = yield function*() {
  try {
    yield fetch(`http://localhost:29000`, { method: "POST", body: "hello" });
  } catch (error) {
    return error;
  }
}
expect(error).toMatchObject({ name: 'FetchError' });

This is more composable (decoupled from context) since it can be abstracted to handle any operation, not just a fetch.

function* capture<T>(operation: Operation<T>): Operation<Error | T> {
  try {
    return yield operation;
  } catch (error) {
    return error;
  }

Which we can then use as

expect(yield capture(fetch(`http://localhost:29000`, { method: "POST", body: "hello" }))).toMatchObject({ name: 'FetchError' });

In fact, I think there might be already a captureError operation in @effection/mocha

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 cd7698e

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

Looks great!

@minkimcello minkimcello merged commit a81ece7 into v2 Jan 25, 2022
@minkimcello minkimcello deleted the mk/fetch branch January 25, 2022 15:02
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.

Update @effection/fetch to depend on cross-fetch ^3.1.5
2 participants