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.

Sign up for GitHub

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

Jump to bottom

fix: write other server build output files #3817

Merged
merged 6 commits into from
Jul 25, 2022
Merged

fix: write other server build output files #3817

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 •
edited
Loading

🦋 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
Assets only imported in resource routes not included in build output #3414
Closed
@mcansh mcansh marked this pull request as ready for review July 21, 2022 20:42
kentcdodds
kentcdodds approved these changes Jul 22, 2022
View reviewed changes
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 mcansh force-pushed the logan/rem-1324-assets-only-imported-in-resource-routes branch from 65f16c7 to 51036c6 Compare July 25, 2022 20:24
@mcansh
test: fix writing files
28fd442 
Signed-off-by: Logan McAnsh <[email protected]>
@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 •
edited
Loading

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

This was referenced Jul 27, 2022
Closed
Closed
Merged
Closed
Merged
mcansh added a commit that referenced this pull request Aug 1, 2022
@mcansh
Revert: dont write other server build output files.
45fe6eb 
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
@mcansh
Revert "fix: write other server build output files (#3817)"
3670cc5 
This reverts commit 916caa6.
@mcansh mcansh mentioned this pull request Jan 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kentcdodds kentcdodds kentcdodds approved these changes

Assignees
No one assigned
Labels
CLA Signed package:dev
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Assets only imported in resource routes not included in build output
4 participants
@mcansh @kentcdodds @wKovacs64 @MichaelDeBoey