-
Notifications
You must be signed in to change notification settings - Fork 18
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: Optionally load gatsby datastore in lambdas #376
feat: Optionally load gatsby datastore in lambdas #376
Conversation
Suspect that the function is completing early because this wasn't happening before
thought this would download the file into the directory, but suspecting that the file needs to exist before the stream writes
aiming to mitigate the effects of the cold start when downloading the gatsby datastore from the CDN
small cleanups
✅ Deploy Preview for netlify-plugin-gatsby-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for ep-optionally-load-gatsby-datastore-in-lambdas ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
if ( | ||
process.env.GATSBY_EXCLUDE_DATASTORE_FROM_BUNDLE === 'true' || | ||
process.env.GATSBY_EXCLUDE_DATASTORE_FROM_BUNDLE === '1' | ||
) { |
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.
tl;dr - Opted to hard code this rather than use shouldSkipBundlingDatastore
because of import issues that result when the lambda functions are compiled.
Longer version
Initially I had changed this to use the shouldSkipBundlingDatastore
method that's in the helpers/config
file. While locally this works, when the functions are built, an import error results because helpers/config
is not included in the lambda bundle.
We could have included the file in the lambda bundle so that the lambda had a reference to shouldSkipBundlingDatastore
, but that didn't make the most sense given that the file mostly contains logic related to the build plugin and the configuration files that should be generated as part of the build process.
We could have also moved the shouldSkipBundlingDatastore
and isEnvSet
methods (on which shouldSkipBundlingDatastore
depends on) into the templates/utils
file, but that also didn't make the most sense since these are configuration related details, not related to templating.
Though not perfect, for the time being this seemed like the simplest/most straightforward solution.
ensureFileSync(filePath) | ||
} | ||
return downloadFile(downloadUrl, filePath) | ||
} | ||
const dir = 'data' |
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.
🤦🏻♂️ oops, sorry
add check for dataMetadata.json file in order to use filename there if it exists
package resolution wasn't picking up fix to netlify/esbuild#17
increase version of node to 14
remove axios as well
this avoids needing to traverse the list of files to reuse the name when possible
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.
A beast of a PR! All LGTM.
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.
Ship it! Great work
For anyone who comes across this in their googling, the environment variable is named |
Summary
Introduce an environment variable that controls whether the Gatsby datastore is bundled in with the lambdas or is uploaded to the CDN and downloaded on first load of the lambdas.
If the environment variable (
LOAD_GATSBY_LMDB_DATASTORE_FROM_CDN
) is enabled, the plugin now also sends a pre-warm request to the SSR, DSG, and API functions. This is because it can take some time to initially download the datastore, and we want to mitigate the impact on our user's end-users of this operation (they could be waiting an extended period of time and it might look like the page is hanging).Additional changes
chance
,uuid
,tmp-package
, andtmp-promise
packages for unit tests14.17.0
due to the use of the AbortController in theonSuccess
build stepexeca
to adevDependency
frompeerDependency
within theplugin/package.json
. This is because we're using it directly within a test, and we also want to ensure that it's on v5 as v6 is entirely in pure ESM which was causing build issues within the project (see release notes)Test plan
Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal
Partially addresses: #320
Doesn't include the following from the linked issue:
Standard checks:
🧪 Once merged, make sure to update the version if needed and that it was
published correctly.