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

when watching entire directory, don't rebuild files outside said directory #2492

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paulcpederson
Copy link
Contributor

#2491 - small fix to the CLI so that non-underscored files outside the watched directory (but imported as part of the tree) are not built.

@xzyfer I've written a test for this with a fixture but skipped it as it looks like you're skipping most of the async tests? Let me know if you want me to add that back in to the list.

@paulcpederson
Copy link
Contributor Author

/cc @Gwerlas

@paulcpederson
Copy link
Contributor Author

@nschonni do you know who could take a look at this?

@saper
Copy link
Member

saper commented Oct 17, 2019

It looks to me that #2491 #2504 #2560 and partially #2479 have the similar root cause.

We constantly maintain a graph of dependencies and we try to rebuild everything on that graph that does not start with underscore.

This is probably not correct - we should maintain two trees:

  • dependency graph
  • expanded original input specification

We should react to all events on the dependency graph, but only files on the original input specification should ever be compiled.

Looking at that code:

node-sass/bin/node-sass

Lines 248 to 252 in 8d0acca

files.changed.forEach(function(file) {
if (path.basename(file)[0] !== '_') {
renderFile(file, options, emitter);
}
});

we should not fire compilation of the changed file as a response to the changed event - only affected original files should be subjected to rendering.

Any thoughts @xzyfer , @nschonni ? If yes, this would mean a major change in our watching code.

jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants