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

fix(compiler): fix file watcher sometimes doesn't trigger rebuild #6191

Merged
merged 2 commits into from
Mar 11, 2025

Conversation

hettiger
Copy link
Contributor

What is the current behavior?

GitHub Issue Number: #6190

The file watcher sometimes fails to trigger a rebuild:

2025-03-11 18 16 49

This is caused by the "Back up files before saving" option in my editor.
When this option is disabled the file watcher works just fine.
However, the "Back up files before saving" option is enabled per default and Stencil should be able to handle this.

The above demo was recorded on a clean install of Stencil. It should be fairly simple to reproduce the issue as long you're using "Back up files before saving". (I don't know if it's just PHPStorm or other editors as well.)

What is the new behavior?

After applying these changes the watcher behaves beautifully with or without "Back up files before saving" option enabled.

Documentation

Does this introduce a breaking change?

  • Yes
  • No

Testing

I tested the code with a custom @stencil/core build in my application running stencil build --watch --verbose. I also made sure that the issue is not specific to my project. I installed a clean Stencil project and repeated my tests with and without the "Back up files before saving" option.

I ran the test suite via npm run test and made sure tests are still passing. I did not add any new tests.

Other information

Ignoring backup files via watchIgnoredRegex: [/\.\w+~$/] or using "**/*.*~" on the watchOptions.excludeFiles tsconfig.json option did not work.

These options generally don't work for me. (Or I'm misinterpreting the NODE_SYS_DEBUG and WATCH_BUILD logging.)

The existing implementation was running into the lastTsBuilder && !timeoutId condition with a timeoutId being present even though there was no running build or any other good reason not to rebuild.

The change simplifies the implementation by using a recursive setTimeout to wait for pending builds.

This allows to:

  • remove the extra rebuildTimer which was arguably confusing
  • remove the clearTimeout override

Since we immediately call clearTimeout(timeoutId) in the setTimeout override, we don't stack up rebuild requests.

The existing implementation was running into the `lastTsBuilder && !timeoutId` condition with a `timeoutId` being present
even though there was no running build or any other good reason not to rebuild.

The change simplifies the implementation by using a recursive `setTimeout` to wait for pending builds.

This allows to:

  - remove the extra `rebuildTimer` which was arguably confusing
  - remove the clearTimeout override

Since we immediately call `clearTimeout(timeoutId)` in the `setTimeout` override, we don't stack up rebuild requests.

After applying these changes the watcher behaves beautifully.

fixes: stenciljs#6190
@hettiger hettiger requested a review from a team as a code owner March 11, 2025 18:15
@hettiger
Copy link
Contributor Author

@christian-bromann Sorry about the formatting error — overlooked the prettier scripts. I fixed it in f977746 using npm run prettier and checked with npm run prettier.dry-run. CI should be passing now.

@christian-bromann
Copy link
Member

Sorry about the formatting error — overlooked the prettier scripts.

No worries at all, happens to me all the time.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@christian-bromann christian-bromann merged commit 840ead9 into stenciljs:main Mar 11, 2025
71 checks passed
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.

2 participants