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

refactor(dev): revert serving assets from dev server #6306

Merged
merged 1 commit into from
May 4, 2023

Conversation

pcattori
Copy link
Contributor

@pcattori pcattori commented May 4, 2023

When sending HMR updates, the assumption was that serving assets from dev server would allow those updates to be sent immediately, without waiting for the app server to restart. But this isn't the case (e.g. if you navigate to another page that has a loader and the app server is rebooting, that loader fetch will fail).


Idea moving forward is:

  1. If you don't mind a bit of downtime from your app server during dev, then you can rely on Remix restarts. but you'll need to wait for app server to reboot before you hard refresh or click buttons that trigger actions, or navigate to different pages that need loaders.

  2. if that amount of downtime is too much for you, using chokidar and purgeRequireCache w/ --no-restart and that will keep you zipping along with an always-up server.

Note: during rebuild there is no downtime, its only after rebuild but before app server restart that there is some downtime with (1)
^not serving assets from dev server also means there's less of a difference b/w dev and prod, which is nice. plus we don't have to use dataurls to handle svgs if we do that.

@changeset-bot
Copy link

changeset-bot bot commented May 4, 2023

⚠️ No Changeset found

Latest commit: bd6b29a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

When sending HMR updates, the assumption was that serving assets from
dev server would allow those updates to be sent immediately, without
waiting for the app server to restart. But this isn't the case (e.g. if
you navigate to another page that has a loader and the app server is
rebooting, that loader fetch will fail).
@pcattori pcattori force-pushed the pedro/refactor-do-not-server-assets-from-dev-server branch from 9133202 to bd6b29a Compare May 4, 2023 18:07
@pcattori pcattori merged commit 0b6bc2c into dev May 4, 2023
@pcattori pcattori deleted the pedro/refactor-do-not-server-assets-from-dev-server branch May 4, 2023 18:31
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-6e23fcf-20230505 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!

@acusti
Copy link
Contributor

acusti commented May 9, 2023

in case this debugging info is useful, i’m using the cloudflare-workers adaptor and found the following when running yarn dev (where "dev": "remix dev --no-restart -c \"wrangler dev --local ./build/index.js\""):

  • using v0.16.0, svg assets 404 (the URL include is http://localhost:3001/…)
  • using v0.0.0-nightly-6e23fcf-20230505 (with this PR), the build gives me errors for every imported svg file: ✘ [ERROR] No loader is configured for ".svg" files: public/name-of-file.svg
  • using v0.0.0-nightly-0e2763f-20230503 (which has the change in fix(dev): serve any non-Remix assets from public/ #6279), svg assets load correctly

@pcattori
Copy link
Contributor Author

@acusti seems like you are also seeing #6316 . I'm unable to reproduce this locally. If you have a repro, could you post it in #6316

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 1.16.1-pre.0 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!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 1.16.1 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
Development

Successfully merging this pull request may close these issues.

4 participants