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

fix: add missing fs method rewrites to handle fetchRemoteFile in dsg/ssr engine #38822

Merged
merged 4 commits into from
Jan 25, 2024

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Jan 24, 2024

Description

This PR fixes issues when fs.createWriteStream (and few others) are being used in functions/lambdas that are mounted in read-only locations

Example error could look like this:

Jan 24, 11:05:05 AM: error [gatsby-source-contentful] Could not getDominantColor from image TypeError: Cannot read properties of undefined (reading 'apply')
Jan 24, 11:05:05 AM: at new WriteStream (/var/task/.cache/query-engine/index.js:6355:29)
Jan 24, 11:05:05 AM: at Object.createWriteStream (/var/task/.cache/query-engine/index.js:6378:12)
Jan 24, 11:05:05 AM: at /var/task/.cache/query-engine/index.js:16689:75
Jan 24, 11:05:05 AM: at new Promise (<anonymous>)
Jan 24, 11:05:05 AM: at requestRemoteNode (/var/task/.cache/query-engine/index.js:16687:10)
Jan 24, 11:05:05 AM: at fetchFile (/var/task/.cache/query-engine/index.js:297107:61)
Jan 24, 11:05:05 AM: at fetchWorker (/var/task/.cache/query-engine/index.js:297015:26)

But any issue (there might be multiple context in which this issue occurs) that looks more or less like this:

TypeError: Cannot read properties of undefined (reading 'apply')
 at new WriteStream (/var/task/.cache/query-engine/index.js:X:Y)
 at Object.createWriteStream (/var/task/.cache/query-engine/index.js:X:Y)

is caused by same root issue which is fact that linked FS (that rewrites path from read-only location to writeable location) didn't cover required FS methods.

This PR adds rewrite handling for:

  • fs.createWriteStream
  • fs.createReadStream
  • fs.rm
  • new fs.WriteStream
  • new fs.ReadStream

And add rewrite support for second (to) argument for ( currently only first (from) argument is being rewritten)

  • fs.rename
  • fs.renameSync

This PR also handles few issues that were discovered during work on missing FS rewrites:

  • E2E test for Remote File (Image/File CDN feature that utilize above FS methods) was not actually being SSR which did hide some problems
  • File CDN url generator was working properly, due to added babel helper that could not be resolved in Netlify Lambda
  • PathPrefix was not set in SSR/DSG engine and URLs generated for Image/File CDN didn't honor it

Tests

Fixed adapter e2e test fixture to actually use SSR for SSR Remote File tests. Enabled placeholder tests for SSR Remote File page (placeholder generation did not work due to missing FS rewrites)

Related Issues

https://linear.app/netlify/issue/FRA-236/fatal-createwritestream-problem-with-linkfs

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jan 24, 2024
@pieh pieh added topic: render-mode Related to Gatsby's different rendering modes topic: media Related to gatsby-plugin-image, or general image/media processing topics and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jan 24, 2024
@@ -29,7 +29,7 @@ const configs = [
{
title: `remote-file (SSR, Page Query)`,
pagePath: `/routes/ssr/remote-file/`,
placeholders: false,
placeholders: true,
Copy link
Contributor Author

@pieh pieh Jan 24, 2024

Choose a reason for hiding this comment

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

This test was disabled because of problems that this PR is fixing. In context of this test those problems weren't fatal, however it meant that image placeholders were just not working.

The fix in this PR also solves other problems than just placeholders, but this was existing test that already we had

@pieh pieh force-pushed the fix/lambda-writestream-linkfs branch from 1d97c5a to b6d9442 Compare January 24, 2024 16:34
@@ -1,4 +1,4 @@
import crypto from "crypto"
import { createHash } from "crypto"
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 caused some problems with

Cannot find module '@babel/runtime/helpers/interopRequireDefault'

in lambdas, so easiest way was just to avoid having babel adding that

Comment on lines +46 to +64
private setupPathPrefix(pathPrefix: string): void {
if (pathPrefix) {
store.dispatch({
type: `SET_PROGRAM`,
payload: {
prefixPaths: true,
},
})

store.dispatch({
type: `SET_SITE_CONFIG`,
payload: {
...store.getState().config,
pathPrefix,
},
})
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed for pathPrefix to be actually honored when generating FILE_CDN urls

Comment on lines +107 to +113
export function getServerData() {
return {
props: {
ssr: true,
},
}
}
Copy link
Contributor Author

@pieh pieh Jan 25, 2024

Choose a reason for hiding this comment

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

This page wasn't actually SSR... Making it SSR uncovered some other problems that this PR is also fixing

@pieh pieh force-pushed the fix/lambda-writestream-linkfs branch from 903659a to c08ed55 Compare January 25, 2024 09:23
@pieh pieh marked this pull request as ready for review January 25, 2024 12:29
// @ts-ignore TS doesn't like extending prototype "classes"
lfs.ReadStream = LinkedReadStream

const dbPath = path.join(TEMP_CACHE_DIR, `data`, `datastore`)

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like there's some repetition here with extending fs.WriteStream and fs.ReadStream, you could consider extracting/reusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's basically the same thing twice (just once for WriteStream and once for ReadStream - I think it's confusing as it is already and trying to create reusable utility would probably make debugging / understanding it worse than it already is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing class ... extends would be much more readable, but you can't actually do that is "class" you extend is prototype one like fs.WriteStream and fs.ReadStream are ( https://github.com/nodejs/node/blob/6b6bcee747c2007117262efb2ff6d61ea888f499/lib/internal/fs/streams.js#L321-L398 being one of them - the other is also in that module)

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, I'm ok leaving as is.

@pieh pieh merged commit bbdddd7 into master Jan 25, 2024
34 checks passed
@pieh pieh deleted the fix/lambda-writestream-linkfs branch January 25, 2024 14:48
pieh added a commit that referenced this pull request Jan 25, 2024
…ssr engine (#38822)

* test(adapters-e2e): enable placeholder tests in ssr remote-file

* fix(gatsby-adapter-netlify): bundling file-cdn

* fix(gatsby): set pathPrefix in engines

* fix(gatsby): add missing fs method rewrites to handle fetchRemoteFile in dsg/ssr engine

(cherry picked from commit bbdddd7)
pieh added a commit that referenced this pull request Jan 25, 2024
…ssr engine (#38822) (#38823)

* test(adapters-e2e): enable placeholder tests in ssr remote-file

* fix(gatsby-adapter-netlify): bundling file-cdn

* fix(gatsby): set pathPrefix in engines

* fix(gatsby): add missing fs method rewrites to handle fetchRemoteFile in dsg/ssr engine

(cherry picked from commit bbdddd7)

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
This was referenced May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: media Related to gatsby-plugin-image, or general image/media processing topics topic: render-mode Related to Gatsby's different rendering modes
Projects
Status: Published
Development

Successfully merging this pull request may close these issues.

2 participants