feat: upload blobs on onDev event #5552
Conversation
ndhoule
force-pushed
the
nathanhoule/ct-651-blobs-client-should-read-from-file-based-upload-folder
branch
from
571c54f
to
2d2f1da
Compare
|
Anyone can review this, but tagging @eduardoboucas as a reviewer because we discussed this functionality. Seems like the failing test check is flaky on main, but let me know if that's not the case and I can dig. |
packages/build/src/steps/get.ts
Outdated
| ...steps, | ||
| // Trigger the uploadBlobs step during development to allow users to test file-based blob | ||
| // uploads locally and to allow frameworks to capture file-based blobs written by frameworks | ||
| { ...uploadBlobs, event: 'onDev' }, |
There was a problem hiding this comment.
I see that you're reusing the core step we have for builds and just changing the event property. As far as I know, we don't really have a precedent for running the same core step in multiple events, but I would probably have two separate core steps — one for dev, one for build — that may reuse common functionality. I think taking a core step and creating another one from it on-the-fly here might be confusing to people.
More importantly, if we run the existing core step as is as part of dev, won't we be uploading blobs to the actual production store? How do we ensure that we're uploading blobs to the local store?
There was a problem hiding this comment.
I see that you're reusing the core step we have for builds and just changing the
eventproperty. As far as I know, we don't really have a precedent for running the same core step in multiple events, but I would probably have two separate core steps — one for dev, one for build — that may reuse common functionality. I think taking a core step and creating another one from it on-the-fly here might be confusing to people.
Sure! I can split this into a dev_blobs_upload core step that uses shared code.
More importantly, if we run the existing core step as is as part of dev, won't we be uploading blobs to the actual production store? How do we ensure that we're uploading blobs to the local store?
As I understand it, the answer to your first question is no: The blobs client used in the upload blobs step picks up the local blob server's settings via process.env['NETLIFY_BLOBS_CONTEXT'], which is set by both the dev and serve commands. (ntl serve actually already uploads blobs to the local blob server: netlify/cli#6299.)
To your second question: I think this concern would apply equally to serve, right? I'm not opposed to adding some checks to ensure serve and dev don't accidentally upload blobs to production, but given that I think we'd already have seen this issue come up in serve, I'm inclined to solve it separately. What do you think?
ndhoule
force-pushed
the
nathanhoule/ct-651-blobs-client-should-read-from-file-based-upload-folder
branch
from
2d2f1da
to
1086316
Compare
|
@eduardoboucas Just pushed up the changes we discussed--let me know what you think! |
| const { | ||
| constants: { IS_LOCAL }, | ||
| } = args[0] | ||
| return IS_LOCAL && ((await uploadBlobs.condition?.(...args)) ?? true) |
There was a problem hiding this comment.
I don't think IS_LOCAL is a particularly useful check given that I wouldn't expect us to run dev anywhere but locally, but it doesn't seem to hurt to perform this check, I guess? I can remove it if you agree it's not useful.
There was a problem hiding this comment.
We're not doing it for other core steps running on the dev timeline, so for consistency we should probably remove it.
| const { | ||
| constants: { IS_LOCAL }, | ||
| } = args[0] | ||
| return IS_LOCAL && ((await uploadBlobs.condition?.(...args)) ?? true) |
There was a problem hiding this comment.
We're not doing it for other core steps running on the dev timeline, so for consistency we should probably remove it.
|
|
||
| export const devUploadBlobs: CoreStep = { | ||
| event: 'onDev', | ||
| coreStep: uploadBlobs.coreStep, |
There was a problem hiding this comment.
Maybe I should've made that clearer, but what I meant by "reuse common functionality" #5552 (comment) was moving shared logic into a separate file that both core steps would use.
In this case, you're importing one core step into the other and reusing the logic it implements. There's no issue with that functionality-wise, but I worry that future us will be changing one core step unaware that it's actually affecting another core step that imports it.
I'm approving because I don't want to block you, but I'd recommend revisiting this if possible.
There was a problem hiding this comment.
I ended up splitting some more common functionality out into the utils/blobs.ts file and otherwise duplicated most of the contents of upload_blobs to dev_upload_blobs.
In general I share your concern, but in practice, it seems like the two steps should behave identically right now. Given that, I'm not sure how best to split out any more common functionality without simply moving the entire core step into a shared file.
I'm going to merge this as it stands so we can address the issue this solves, but if you have suggestions on how we might better structure this, let me know--happy to address it in a follow-up PR!
This changeset updates the dev timeline's steps to run the `uploadBlobs` core plugin on the `onDev` event. (It will also allow users to use file- based blobs locally using `netlify dev`, but that's an ancillary benefit.) Some background on the "why" of this issue: Frameworks has been exploring use cases that involve writing to the blobs directory (for example, a Remix site that writes an initial cache static files into the blobs directory that may later be invalidated and replaced with dynamically generated and served assets). They gave the feedback that it's difficult to test out this type of functionality locally, where `netlify dev` is their primary workflow. Fixes [CT-651](https://linear.app/netlify/issue/CT-651).
ndhoule
force-pushed
the
nathanhoule/ct-651-blobs-client-should-read-from-file-based-upload-folder
branch
from
85a607c
to
67ba719
Compare
This shares some, but not all, functionality between the two tasks. We should figure out how to better share functionality between the two jobs but this should be fine for now.
ndhoule
force-pushed
the
nathanhoule/ct-651-blobs-client-should-read-from-file-based-upload-folder
branch
from
67ba719
to
da6a5eb
Compare
|
@eduardoboucas Looks like I need a re-review to merge 🙏 |
…from-file-based-upload-folder
…from-file-based-upload-folder
ndhoule
deleted the
nathanhoule/ct-651-blobs-client-should-read-from-file-based-upload-folder
branch
This changeset updates the dev timeline's steps to run the
uploadBlobscore plugin on theonDevevent. (It will also allow users to use file- based blobs locally usingnetlify dev, but that's an ancillary benefit.)Some background on the "why" of this issue: Frameworks has been exploring use cases that involve writing to the blobs directory (for example, a Remix site that writes an initial cache static files into the blobs directory that may later be invalidated and replaced with dynamically generated and served assets). They gave the feedback that it's difficult to test out this type of functionality locally, where
netlify devis their primary workflow.I'm not married to this particular implementation, but this seemed like the simplest way to accomplish it. Also, I wasn't sure where would be best to add tests for this functionality--happy to add them with some direction!
Fixes CT-651.