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

Update chokidar to watch version.txt #450

Merged
merged 1 commit into from Sep 16, 2023
Merged

Conversation

xHomu
Copy link
Contributor

@xHomu xHomu commented Sep 16, 2023

Update chokidar to resolve a rare race condition issue that resultant in dev server crashing. This PR remix-run/remix#7299 sets up a fix where we can watch the version.txt file for changes (rather than build/index.js directly) to avoid race condition.

Test Plan

This affects dev server behavior only.

Checklist

  • Tests updated
  • Docs updated

Screenshots

In some rare circumstances, the current chokidar watch setup could result in the following error:

/.../node_modules/@remix-run/server-runtime/dist/dev.js:26
      buildHash: build.assets.version
                              ^
TypeError: Cannot read properties of undefined (reading 'version')
    at Object.broadcastDevReady (/.../node_modules/@remix-run/server-runtime/dist/dev.js:26:31)
    at FSWatcher.handleServerUpdate (/.../node_modules/@remix-run/serve/dist/cli.js:74:12)

We sidesteped this race condition issue by introducing a new file in Remix v2, version.txt, that only gets updated after the build completes.

References

Update chokidar to resolve a rare race condition issue that resultant in dev server crashing. This PR remix-run/remix#7299 sets up a fix where we can watch the version.txt file for changes (rather than build/index.js directly) to avoid race condition.
@xHomu xHomu changed the title Update server.ts to v2 best practices https://github.com/remix-run/remix/issues/6919 Update server.ts to watch version.txt Sep 16, 2023
@xHomu xHomu changed the title Update server.ts to watch version.txt Update chokidar to watch version.txt Sep 16, 2023
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.

Fantastic improvements. Thank you!

@kentcdodds kentcdodds merged commit 4626d54 into epicweb-dev:main Sep 16, 2023
5 checks passed
@xHomu xHomu deleted the patch-1 branch September 16, 2023 17:55
@xHomu
Copy link
Contributor Author

xHomu commented Sep 16, 2023

Ops, chokidar should be a devDependency now.

@kentcdodds
Copy link
Member

Fixed 👍

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

Successfully merging this pull request may close these issues.

None yet

2 participants