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

Amend examples and docs to reflect that chokidar watcher needs to be closed for build to finish #1075

Closed

Conversation

lostfictions
Copy link

Description

I discovered -- after much confusion -- that if you have a persistent chokidar watcher set up in your static.config.js and you try to run react-static build, the build will successfully complete, but then hang forever. This PR amends the examples and docs to reflect that the watcher needs to be closed in onBuild.

Doesn't look like this repo has any integration tests, otherwise I would've added one for verifying that a build that uses chokidar doesn't time out.

It looks like at least one or two of these examples are bit by #985, so this won't fundamentally make them work -- changing any content will still completely break react-static and will require a server restart to make things work again. But at least builds will work on CI (and won't require being ctrl-c'd out of locally). fwiw #985 seems like a serious, critical bug and I'm not sure react-static is usable in a lot of scenarios until it's fixed.

On another note, I'm not sure why the examples were all "deprecated" in one fell swoop -- many of them contain useful info that still isn't available in guides. I saw the explanation in the docs about why this happened, but I also think that examples and guides serve complementary usecases; some people find it easier to get started from one or the other. (next.js's examples directory might seem a little extreme, but I really appreciate that they put work in to keep all of them up-to-date and to explain the motivation behind each one.)

Changes/Tasks

  • Changed code
  • Added comment to usage of chokidar in node-api.md

Types of changes

  • Refactoring/add tests (refactoring or adding test which isn't a fix or add a feature)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • My changes have tests around them

@tannerlinsley
Copy link
Contributor

You may want to check out the changes being done in v7 for loading and watching routes from a directory before we move forward.

  • The issue may already be resolved with those changes in v7, or your PR might be able to improve these changes
  • I am so happy you figured out that this is why things are hanging. It makes a ton of sense now!
  • The examples directory was nice for some things, but honestly it became a logistical problem. Unfortunately I am still the only very active maintainer of RS and it was simply too much technical debt. I still have them saved in the repo in https://github.com/nozzle/react-static/tree/v7/archives/old-examples. This has helped keep things maintainable for me, but I see your point.

@lostfictions
Copy link
Author

Thanks for the reply! I took a look at the v7 tree -- all those changes sound great. If the new source-directory plugin is just for loading and watching routes, I don't think it impacts this PR -- these changes are for the scenarios where a directory of markdown files gets watched so that their data can be passed down to routes, regardless of where or how they're defined. Assuming I'm not misunderstanding the use of the plugin?

@tannerlinsley
Copy link
Contributor

Yeah, this shouldn't affect v7 or any new stuff. If you could, fix up this PR with master again so we can get it merged without conflicts?

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