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

Watch/poll missing changes when files change #2440

Open
hcker2000 opened this issue Nov 22, 2024 · 8 comments · May be fixed by #2444
Open

Watch/poll missing changes when files change #2440

hcker2000 opened this issue Nov 22, 2024 · 8 comments · May be fixed by #2444
Labels
bug CLI Issues about the command-line tools

Comments

@hcker2000
Copy link

To be more specific when switching git branches with sass --watch or sass --watch --poll it will miss the new @use lines inside the sass file that's supposed to be being watched.

For example if we add the following to branch new-fonts:

@use newfonts

Then switch back to the master branch it will still try to the @use line even though the file and line no longer exist.

@nex3
Copy link
Contributor

nex3 commented Nov 22, 2024

Unfortunately there's not much we can do about this. Sass relies on the operating system and intermediate libraries to notify it when files change on disk, and there's no way for it to tell when that stack is feeding us inaccurate information. You can always use the --poll option, which will be slower in general but more accurate since it just checks the filesystem directly instead of relying on OS notifications.

@nex3 nex3 closed this as not planned Won't fix, can't repro, duplicate, stale Nov 22, 2024
@hcker2000
Copy link
Author

hcker2000 commented Nov 22, 2024 via email

@nex3 nex3 reopened this Nov 23, 2024
@nex3 nex3 added needs info CLI Issues about the command-line tools labels Nov 23, 2024
@nex3
Copy link
Contributor

nex3 commented Nov 23, 2024

Okay, that could be an issue on our end. How have you installed Dart Sass? Can you provide a repo along with minimal steps to consistently reproduce the issue?

@hcker2000
Copy link
Author

hcker2000 commented Nov 23, 2024 via email

@hcker2000
Copy link
Author

hcker2000 commented Nov 25, 2024

Ok this replicates the issue: https://github.com/hcker2000/dart-sass-debug

If you checkout v2 and then main and then v2 you will see what happens, it looks like the logic is almost inverted as i am seeing this when I switch back to main from v2:

body {
  background-color: #0f0;
}

body div {
  font-weight: 800;
}

/*# sourceMappingURL=data:application/json;charset=utf-8,%7B%22version%22:3,%22sourceRoot%22:%22%22,%22sources%22:%5B%22_child.sass%22,%22_other.sass%22%5D,%22names%22:%5B%5D,%22mappings%22:%22AAAA;EACI;;;ACAA;EACI%22,%22file%22:%22output.css%22%7D */

It should be:

body {
  background-color: #0f0;
}

/*# sourceMappingURL=data:application/json;charset=utf-8,%7B%22version%22:3,%22sourceRoot%22:%22%22,%22sources%22:%5B%22_child.sass%22%5D,%22names%22:%5B%5D,%22mappings%22:%22AAAA;EACI%22,%22file%22:%22output.css%22%7D */

If you add a blank line to main and then save it when the main branch is checked out it will re-compile and be fine.

@nex3
Copy link
Contributor

nex3 commented Nov 25, 2024

I can't reproduce. When I run sass --watch --poll src/main.sass:src/main.css and switch back and forth between the main and v2 branches, the file gets updated correctly after a second or so each time.

Note that nothing in this repo actually loads _other.sass, so body div { font-weight: 800 } is never emitted.

@hcker2000
Copy link
Author

hcker2000 commented Nov 25, 2024

Sorry I missed pushing a commit on the v2 branch. pull and try now. I do know that the console shows that it re-compiles but ends up not being correct.

@nex3
Copy link
Contributor

nex3 commented Nov 25, 2024

Okay, that lets me reproduce it. This is indeed our bug. Here's what's happening:

  • You check out a new branch.
  • The polling watcher reports that _child.sass has changed.
  • Sass knows this means it needs to recompile main.sass, so it does. But as far as it knows, main.sass hasn't changed, so it uses its cached AST instead of re-reading the file.
  • The polling watcher reports that main.sass has changed.
  • Sass checks the modification time of main.css, sees that it's newer than main.sass (because it was just recompiled above), and doesn't recompile.

We do debounce events to avoid situations like this, but it looks like our debounce timeout is faster than the polling watcher's delay in reports so it's not working here. It's possible we should also add some additional mtime checks to avoid using stale cached ASTs.

@nex3 nex3 added bug and removed needs info labels Nov 25, 2024
nex3 added a commit that referenced this issue Nov 26, 2024
1. The import cache now tracks the most recent time it actually loaded
   a stylesheet, so that `--watch` mode can invalidate cached data
   if it's older than the mtime on disk. This should help catch some
   cases where the watcher information doesn't match the filesystem.

2. Rather than eagerly recompiling as soon as it knows it needs to,
   the stylesheet graph now only starts recompiling once it's finished
   processing a batch of events. This ensures that any cache
   invalidation is finished before the recompilation happens.

3. The stylesheet graph and import cache now eagerly invalidate all
   canonicalize results that could be changed by an added or removed
   file, rather than only those that are implicated by the in-memory
   ASTs. This avoids issues when the in-memory AST is stale.

Closes #2440
nex3 added a commit that referenced this issue Nov 26, 2024
1. The import cache now tracks the most recent time it actually loaded
   a stylesheet, so that `--watch` mode can invalidate cached data
   if it's older than the mtime on disk. This should help catch some
   cases where the watcher information doesn't match the filesystem.

2. Rather than eagerly recompiling as soon as it knows it needs to,
   the stylesheet graph now only starts recompiling once it's finished
   processing a batch of events. This ensures that any cache
   invalidation is finished before the recompilation happens.

3. The stylesheet graph and import cache now eagerly invalidate all
   canonicalize results that could be changed by an added or removed
   file, rather than only those that are implicated by the in-memory
   ASTs. This avoids issues when the in-memory AST is stale.

Closes #2440
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug CLI Issues about the command-line tools
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants