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(remix-dev): write other server build output files to assetsBuildDirectory #3841

Conversation

mcansh
Copy link
Collaborator

@mcansh mcansh commented Jul 26, 2022

(cherry picked from commit 6ae5cdf)

Signed-off-by: Logan McAnsh [email protected]

closes #3414
closes #3415

Closes: #

  • Docs
  • Tests

Testing Strategy:

Signed-off-by: Logan McAnsh <[email protected]>
(cherry picked from commit 6ae5cdf)
@changeset-bot
Copy link

changeset-bot bot commented Jul 26, 2022

🦋 Changeset detected

Latest commit: 8dea114

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
remix Patch
@remix-run/dev Patch
create-remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/vercel Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kentcdodds
Copy link
Member

Ok, to make sure I understand, we're now writing static files to both the public directory as well as the server build directory because those static files can be accessed by both sides of the app right?

I wonder if there's a way for us to determine which side of the app needs it. I'm worried that we'd have issues in serverless situations where you're limited on the amount of content you can upload 🤔

@mcansh
Copy link
Collaborator Author

mcansh commented Jul 26, 2022

Ok, to make sure I understand, we're now writing static files to both the public directory as well as the server build directory because those static files can be accessed by both sides of the app right?

correct

I wonder if there's a way for us to determine which side of the app needs it. I'm worried that we'd have issues in serverless situations where you're limited on the amount of content you can upload 🤔

totally a valid concern, I'll check it out

edit: looks like we only get back path, contents (bytes), and text (content as string)
edit 2: i think the metafile has what we want
edit 3: jk that's only server things, so we won't know the client files - and doesn't include output files

Signed-off-by: Logan McAnsh <[email protected]>
@kentcdodds
Copy link
Member

Let's ask @jacob-ebey about this. I have a feeling he'd know the implications of us copying these files over better.

@mcansh mcansh linked an issue Jul 29, 2022 that may be closed by this pull request
@ni554n
Copy link
Contributor

ni554n commented Aug 1, 2022

Hi, is this going fix the extraneous tailwind .css files generating in functions/_assets directory?

that's not the behavior we wanted, we instead wanted these files to end up in the clinet build assetsDirectory

Revert "fix: write other server build output files (#3817)"

This reverts commit 916caa6.
@mcansh
Copy link
Collaborator Author

mcansh commented Aug 22, 2022

Hi, is this going fix the extraneous tailwind .css files generating in functions/_assets directory?

it will, yes, sorry about that 😬

@MichaelDeBoey MichaelDeBoey changed the title fix: write other server build output files to assetsBuildDirectory fix(remix-dev): write other server build output files to assetsBuildDirectory Aug 29, 2022
@houmark
Copy link
Contributor

houmark commented Sep 10, 2022

Just adding my concern with this one. Cloudflare workers are very limited in size, so a few assets copied to the server build directory could easily block building I think.

@mcansh
Copy link
Collaborator Author

mcansh commented Sep 13, 2022

Just adding my concern with this one. Cloudflare workers are very limited in size, so a few assets copied to the server build directory could easily block building I think.

valid concern, the only extra files that should be written to the build directory would be those imported in resource routes

@houmark
Copy link
Contributor

houmark commented Sep 26, 2022

valid concern, the only extra files that should be written to the build directory would be those imported in resource routes

Thanks @mcansh, but not 100% sure I understand that reply. If I have image files, for example, inside a resource route, will those be imported into the build directory, and will those be part of the cloud function itself, meaning it will count to a potential limit of that cloud service (ie. currently 1MB at Cloudflare)?

New to Remix and new to Cloudflare Pages, so just want to understand what one can expect here. I know best practice is to use a static asset storage, so one would not want to add many image files as imports, but there can be some situations where that is needed.

@mcansh
Copy link
Collaborator Author

mcansh commented Sep 27, 2022

If I have image files, for example, inside a resource route, will those be imported into the build directory, and will those be part of the cloud function itself, meaning it will count to a potential limit of that cloud service (ie. currently 1MB at Cloudflare)?

they will end up in the public/build directory, yes, but for CF, they'll be uploaded to KV just like your other assets so it shouldn't affect your function size, currently if you import a file only in a resource route, that file is completely ignored.

@mcansh mcansh merged commit 3060234 into dev Sep 27, 2022
@mcansh mcansh deleted the logan/rem-1324-write-assets-only-imported-in-resource-routes-to-assetsdirectory branch September 27, 2022 16:27
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-b1b3759-20220928 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants