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

feat(remix-dev,remix-serve): include publicPath in server build manifest #3349

Merged

Conversation

mcansh
Copy link
Collaborator

@mcansh mcansh commented May 31, 2022

this allows remix dev and remix-serve [build-dir] to use your customized assetsBuildDirectory and publicPath.

in remix dev we already read the config, so we can just pass config.publicPath and config.assetsBuildDirectory along to remix-serve's createApp function to use, but in remix-serve [build-dir] we'll require the passed buildPath in remix-serve/cli.ts and pass those from the build itself to createApp

@mcansh mcansh force-pushed the logan/rem-1118-include-publicpath-in-server-manifest branch 2 times, most recently from 3672bbe to 909ec93 Compare May 31, 2022 19:01
@MichaelDeBoey MichaelDeBoey changed the title feat: include publicPath in server build manifest feat: include publicPath in server build manifest May 31, 2022
@mcansh mcansh changed the title feat: include publicPath in server build manifest feat(remix-dev): include publicPath in server build manifest Jun 1, 2022
@mcansh mcansh marked this pull request as ready for review June 3, 2022 15:48
@mcansh mcansh force-pushed the logan/rem-1118-include-publicpath-in-server-manifest branch from f9e87fb to 40a8fe3 Compare June 22, 2022 18:38
@mcansh mcansh changed the title feat(remix-dev): include publicPath in server build manifest feat(remix-serve): allow customizing assets directory, public path, and port Jun 22, 2022
packages/remix-serve/cli.ts Outdated Show resolved Hide resolved
@mcansh mcansh changed the title feat(remix-serve): allow customizing assets directory, public path, and port feat(remix-serve): add flags for various things Jun 23, 2022
@mcansh mcansh changed the title feat(remix-serve): add flags for various things feat(remix-serve): add flags for various options Jun 23, 2022
@mcansh mcansh changed the title feat(remix-serve): add flags for various options feat(remix-dev,remix-serve): include publicPath in server build manifest Jun 23, 2022
@ryanflorence
Copy link
Member

Looks good, we don't really have testing setup for remix-serve (in dev or production), how did you verify this all works?

Any idea why some of the tests are failing?

@mcansh
Copy link
Collaborator Author

mcansh commented Jul 1, 2022

Looks good, we don't really have testing setup for remix-serve (in dev or production), how did you verify this all works?

  1. npm run playground:new
  2. change publicPath and assetsBuildDirectory in remix config
  3. node ./node_modules/@remix-run/dev/dist/cli.js dev
  4. node ./node_modules/@remix-run/dev/dist/cli.js build
  5. node ./node_modules/@remix-run/serve/dist/cli.js build

I'll reverify again just to be sure, but we could also cut an experimental just to make sure that all works as remix dev and remix-serve

Any idea why some of the tests are failing?

i changed the manifest and didn't update the snapshots 😬

@mcansh
Copy link
Collaborator Author

mcansh commented Jul 1, 2022

confirmed running locally using node ... works both in development (both remix watch and remix dev) and production
image

@mcansh
Copy link
Collaborator Author

mcansh commented Jul 5, 2022

published an experimental (0.0.0-experimental-6f3ef540b) and it works for both remix dev and remix-serve [build-dir]
image

image

this allows `remix dev` and `remix-serve [build-dir]` to use your customized `assetsBuildDirectory` and `publicPath`

Signed-off-by: Logan McAnsh <logan@mcan.sh>
@mcansh mcansh force-pushed the logan/rem-1118-include-publicpath-in-server-manifest branch from 6f3ef54 to 67fbd40 Compare July 6, 2022 15:54
@ryanflorence ryanflorence merged commit 55bb00e into dev Jul 11, 2022
@ryanflorence ryanflorence deleted the logan/rem-1118-include-publicpath-in-server-manifest branch July 11, 2022 17:46
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-de9fc05-20220712 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!

mcansh added a commit that referenced this pull request Jul 14, 2022
x-ref #3349

Signed-off-by: Logan McAnsh <logan@mcan.sh>
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.

None yet

3 participants