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

feat(gatsby-core-utils): improve fetch-remote-file #34758

Merged
merged 9 commits into from
Feb 17, 2022

Conversation

wardpeet
Copy link
Contributor

@wardpeet wardpeet commented Feb 8, 2022

Description

Improving remote fetch by adding two bits:

  1. proper mutex so we can't have race conditions - powered by lmdb
  2. have a more persistent store for file downloads that can be persistent

fetch-remote-file is now decoupled from gatsby's cache. It still functions roughly the same but if you give it a proper cacheKey like contentdigest of a node we can keep the file around between builds even if package.json or gatsby got changed.

Documentation

FetchRemoteFile is not documented yet, will do that afterwards.

TODO

  • Add integration test

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 8, 2022
@wardpeet wardpeet changed the title feat(gatsby-core-utils): create proper mutex feat(gatsby-core-utils): improve fetch-remote-file Feb 8, 2022
@wardpeet wardpeet added topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Feb 8, 2022
@wardpeet wardpeet changed the base branch from master to feat/mutex February 8, 2022 16:11
@wardpeet wardpeet force-pushed the feat/remote-file-update branch from 9219a41 to c221936 Compare February 8, 2022 16:12
@@ -0,0 +1,269 @@
import fs from "fs-extra"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file stayed untouched

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed a few things after all

@wardpeet wardpeet force-pushed the feat/remote-file-update branch from c221936 to c862f5a Compare February 8, 2022 18:57
@wardpeet wardpeet marked this pull request as ready for review February 8, 2022 18:57
@wardpeet wardpeet force-pushed the feat/remote-file-update branch from c862f5a to 3a30cf4 Compare February 8, 2022 20:39
@@ -380,316 +363,6 @@ describe(`fetch-remote-file`, () => {
expect(gotStream).toBeCalledTimes(1)
})

it(`only writes the file once when multiple workers fetch at the same time`, async () => {
Copy link
Contributor Author

@wardpeet wardpeet Feb 8, 2022

Choose a reason for hiding this comment

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

Moving this to integration test

@wardpeet wardpeet force-pushed the feat/remote-file-update branch 2 times, most recently from 7114493 to 7d4a00e Compare February 9, 2022 07:50
jest.config.js Outdated Show resolved Hide resolved
// Called if we stall for 30s without receiving any data
const handleTimeout = async (): Promise<void> => {
fsWriteStream.close()
await fs.remove(tmpFilename)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

made this async instead of sync for better windows support

Copy link
Contributor

Choose a reason for hiding this comment

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

What the catch with sync fs.remove on windows?

}

fsWriteStream.close()
await fs.remove(tmpFilename)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

made this async instead of sync for better windows support


// We have an incomplete download
if (!haveAllBytesBeenWritten) {
await fs.remove(tmpFilename)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

made this async instead of sync for better windows support

@wardpeet wardpeet force-pushed the feat/remote-file-update branch 5 times, most recently from f813d2a to b564e98 Compare February 15, 2022 21:13
Base automatically changed from feat/mutex to master February 16, 2022 11:15
@wardpeet wardpeet force-pushed the feat/remote-file-update branch from 0412bac to 56b467b Compare February 16, 2022 14:47
mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
* feat(gatsby-core-utils): improve fetch-remote-file

* feat: use cacheKey in fetch-remote-file

* use async file operations

* Improve tests + low hanging fruit

* revert wordpress

* feat: make sure 304 works

* fix jest config wordpress

* add integration test

* update tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants