pmcelhaney / counterfact

OpenAPI / Swagger to TypeScript generator and mock server
MIT License
83 stars 12 forks source link

reload everything when a file changes #865

Closed pmcelhaney closed 1 month ago

pmcelhaney commented 1 month ago

Fixes #835

When any file is touched, the entire app is reloaded. This is necessary because any file that imports the file that was changed (directly or indirectly) needs to be reloaded.

I'm a bit concerned this is going to cause performance problems on large codebases, but we'll see. Ideally, instead of reloading everything, we would trace dependencies and only reload the files that need to be reloaded.

changeset-bot[bot] commented 1 month ago

⚠️ No Changeset found

Latest commit: ee8d39318905e80a175211e3c8df57d96c71fa38

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

coveralls commented 1 month ago

Pull Request Test Coverage Report for Build 8836858861

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/server/context-registry.ts 7 9 77.78%
<!-- Total: 59 61 96.72% -->
Files with Coverage Reduction New Missed Lines %
src/server/module-loader.ts 1 93.33%
src/server/registry.ts 2 99.06%
<!-- Total: 3 -->
Totals Coverage Status
Change from base Build 8823221097: 0.6%
Covered Lines: 2925
Relevant Lines: 3384

💛 - Coveralls
dethell commented 1 month ago

Fixes #835

When any file is touched, the entire app is reloaded. This is necessary because any file that imports the file that was changed (directly or indirectly) needs to be reloaded.

I'm a bit concerned this is going to cause performance problems on large codebases, but we'll see. Ideally, instead of reloading everything, we would trace dependencies and only reload the files that need to be reloaded.

I'm going to pull this branch and test against our codebase which is pretty large. Should know the results here in the next hour.

dethell commented 1 month ago

I'm going to pull this branch and test against our codebase which is pretty large. Should know the results here in the next hour.

I can't boot up with this change. I'll PM you a sample openapi file to see if it fails on your end as well.

pmcelhaney commented 1 month ago

I'm going to pull this branch and test against our codebase which is pretty large. Should know the results here in the next hour.

I can't boot up with this change. I'll PM you a sample openapi file to see if it fails on your end as well.

Did you check main to make sure it's broken in this change and not another recent change?

dethell commented 1 month ago

Here are some findings using DEBUG=counterfact:*:

It looks like it is stuck in an endless reloading loop. It will say "loading: /Users/davidethell/source/whatever/mock-server/api/.cache' over and over and then say "finished loading: " with the same path and just keep going back and forth between the two. Clearing the .cache folder doesn't help.

pmcelhaney commented 1 month ago

Thanks. That log message wasn't very useful because it was logging the root directory instead of the subdirectory that's being loaded. Pushed an update. Try it now.

dethell commented 1 month ago

Thanks. That log message wasn't very useful because it was logging the root directory instead of the subdirectory that's being loaded. Pushed an update. Try it now.

Trying now. interestingly enough, it did finally finish booting up and showing the REPL prompt. Not sure how long it took, maybe 20 minutes? I was on a meeting not watching it closely, but it did load.

Your update you pushed does show more detail and it confirms it just keeps looping through the same paths.

pmcelhaney commented 1 month ago

I just pushed an update that ignores files with syntax errors. I was hoping that would be the fix.

I think the issue may be that watch() ends up calling load() again for every single file.

dethell commented 1 month ago

I just pushed an update that ignores files with syntax errors. I was hoping that would be the fix.

I think the issue may be that watch() ends up calling load() again for every single file.

Still looping as before.

pmcelhaney commented 1 month ago

Closing this PR. I'm going to trace dependencies and reload only the affected files instead.