-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update node-fetch in the API to fix connection drop issue #1397
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats - desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Bundle Stats - loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Oops.. you added the |
Good catch! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that fixed connection issues to instances of actual-server running on fly.io
packages/api/test.js
Outdated
@@ -1,8 +1,9 @@ | |||
import * as api from './index'; | |||
let api = require('./dist/index'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests shouldn't rely on the compiled code, but instead the source. Any way we can change this back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this should be testing the compiled code to make sure that it actually behaves correctly. It isn’t automatically run at this point (#1074) but running directly against the source somehow could give us false confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually.. this file is not even used. So I would vote to kill it off entirely.
Even when we add proper tests to api: it won't really make much sense since it's not really a unit test or a e2e test. Just some weird leftover I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
Can you do a release? (Also fine to wait if you want to do it with the other August releases) |
Lets do it with August release. |
We’ll now hint that the file ID might be incorrect if that’s the type of error we get.
Fixes #1394 by updating
node-fetch
(unconfirmed)