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: write other server build output files #3817

Merged
merged 6 commits into from
Jul 25, 2022

Conversation

mcansh
Copy link
Collaborator

@mcansh mcansh commented Jul 21, 2022

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Jul 21, 2022

🦋 Changeset detected

Latest commit: 28fd442

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/eslint-config Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/cloudflare Patch
@remix-run/node Patch
@remix-run/deno Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/vercel Patch
@remix-run/architect 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

@mcansh mcansh force-pushed the logan/rem-1324-assets-only-imported-in-resource-routes branch from 263d682 to 164a533 Compare July 21, 2022 20:41
@mcansh mcansh linked an issue Jul 21, 2022 that may be closed by this pull request
@mcansh mcansh marked this pull request as ready for review July 21, 2022 20:42
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Solid 👍

integration/resource-routes-test.ts Outdated Show resolved Hide resolved
@kentcdodds
Copy link
Member

This is good to merge when the tests are green 👍

mcansh and others added 5 commits July 25, 2022 16:11
@mcansh mcansh force-pushed the logan/rem-1324-assets-only-imported-in-resource-routes branch from 65f16c7 to 51036c6 Compare July 25, 2022 20:24
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@mcansh mcansh merged commit 916caa6 into dev Jul 25, 2022
@mcansh mcansh deleted the logan/rem-1324-assets-only-imported-in-resource-routes branch July 25, 2022 20:52
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-916caa6-20220726 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!

@wKovacs64
Copy link
Contributor

While this does produce the files in the build output on the file system, for some reason they don't seem to be picked up by express.static middleware and the request for the asset falls through to the Remix request handler.

Note: my current workaround of re-importing the assets in a UI route still works, in case that provides any additional insight.

@kentcdodds
Copy link
Member

@wKovacs64 can you create a reproduction and open an issue?

@wKovacs64
Copy link
Contributor

@wKovacs64 can you create a reproduction and open an issue?

I wrote the original issue and the repro PR. Are you requesting another one or are these sufficient?

@kentcdodds
Copy link
Member

Ah, sorry, I didn't notice those. @mcansh can you look into why these changes didn't address the original issue?

@mcansh
Copy link
Collaborator Author

mcansh commented Jul 26, 2022

Ah, sorry, I didn't notice those. @mcansh can you look into why these changes didn't address the original issue?

i think i misread/misinterpreted the initial bug report, but the reason for them not being in the public/build and only /build is due to them only being server assets - i'll take a look, but i'm not 100% sure why you'd want to import something in a recorded route and use it elsewhere.

edit: ah didn't see the pr with test case, yeah that usage makes sense, i'll dig in

@mcansh
Copy link
Collaborator Author

mcansh commented Jul 26, 2022

alrighty, should be resolved here: #3841

i'm writing files to both the serverBuildPath (esbuild default) and assetsDirectory

mcansh added a commit that referenced this pull request Aug 1, 2022
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 added a commit that referenced this pull request Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assets only imported in resource routes not included in build output
4 participants