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

Watcher should track newly created files #1896

Closed
wants to merge 2 commits into from

Conversation

xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Feb 13, 2017

Currently newly created are added to the graph but not added the
watcher.

Fixes #1891

@xzyfer xzyfer self-assigned this Feb 13, 2017
@xzyfer xzyfer force-pushed the watcher-added branch 4 times, most recently from 68d52b3 to b12002a Compare February 14, 2017 12:53
@xzyfer
Copy link
Contributor Author

xzyfer commented Feb 14, 2017

I've digging into the watcher tests. I don't think I've addressed all the flakiness but I'm confident I can now address the flaky specs as we notice them.

I've reenabled them as part of this PR.

@xzyfer
Copy link
Contributor Author

xzyfer commented Feb 22, 2017

Some watcher tests still appear to be flakey. I've done a little more hardening. I'll re-disable any particularly troublesome specs until I have a better solution. Keen to get this out soon.

@xzyfer xzyfer force-pushed the watcher-added branch 6 times, most recently from a4ddbdb to 731a73c Compare February 23, 2017 13:34
@gaearon
Copy link

gaearon commented Feb 24, 2017

Thanks! Sorry that getting it through seems frustrating 😞 Hope you’ll figure it out!

@gaearon
Copy link

gaearon commented Mar 28, 2017

Is there anything here that community could help you with?

@xzyfer
Copy link
Contributor Author

xzyfer commented Mar 28, 2017 via email

@xzyfer xzyfer force-pushed the watcher-added branch 12 times, most recently from 3e19b54 to bdde742 Compare March 30, 2017 12:35
@xzyfer
Copy link
Contributor Author

xzyfer commented Mar 30, 2017

I think I'm really close on this now. Should hopefully land this weekend.

Currently newly created are added to the graph but not added the
watcher.

Fixes sass#1891
These have been notoriously flakey so they were disabled a long ago.
xzyfer added a commit to xzyfer/node-sass that referenced this pull request Jun 15, 2017
This logic is all tightly coupled in the bin. This logic has been
notoriously impossible to test due to weird fs timing issues. By
extracting the actual logic we're now able to test it in isolation.

With this in place replacing Gaze (sass#636) becomes a viable option.

Closes sass#1896
Fixes sass#1891
xzyfer added a commit to xzyfer/node-sass that referenced this pull request Jun 15, 2017
This logic is all tightly coupled in the bin. This logic has been
notoriously impossible to test due to weird fs timing issues. By
extracting the actual logic we're now able to test it in isolation.

With this in place replacing Gaze (sass#636) becomes a viable option.
This PR not only massively increases our test coverage for the
watcher but also address a bunch of known edge cases i.e. orphaned
imports when a files is deleted.

Closes sass#1896
Fixes sass#1891
xzyfer added a commit to xzyfer/node-sass that referenced this pull request Jun 15, 2017
This logic is all tightly coupled in the bin. This logic has been
notoriously impossible to test due to weird fs timing issues. By
extracting the actual logic we're now able to test it in isolation.

With this in place replacing Gaze (sass#636) becomes a viable option.
This PR not only massively increases our test coverage for the
watcher but also address a bunch of known edge cases i.e. orphaned
imports when a files is deleted.

Closes sass#1896
Fixes sass#1891
xzyfer added a commit to xzyfer/node-sass that referenced this pull request Jun 15, 2017
This logic is all tightly coupled in the bin. This logic has been
notoriously impossible to test due to weird fs timing issues. By
extracting the actual logic we're now able to test it in isolation.

With this in place replacing Gaze (sass#636) becomes a viable option.
This PR not only massively increases our test coverage for the
watcher but also address a bunch of known edge cases i.e. orphaned
imports when a files is deleted.

Closes sass#1896
Fixes sass#1891
xzyfer added a commit to xzyfer/node-sass that referenced this pull request Jun 15, 2017
This logic is all tightly coupled in the bin. This logic has been
notoriously impossible to test due to weird fs timing issues. By
extracting the actual logic we're now able to test it in isolation.

With this in place replacing Gaze (sass#636) becomes a viable option.
This PR not only massively increases our test coverage for the
watcher but also address a bunch of known edge cases i.e. orphaned
imports when a files is deleted.

Closes sass#1896
Fixes sass#1891
@xzyfer
Copy link
Contributor Author

xzyfer commented Jun 16, 2017

Tacking this in #2020. Should land any day now.

@xzyfer xzyfer closed this Jun 16, 2017
@xzyfer xzyfer deleted the watcher-added branch June 16, 2017 23:51
xzyfer added a commit that referenced this pull request Jun 17, 2017
This logic is all tightly coupled in the bin. This logic has been
notoriously impossible to test due to weird fs timing issues. By
extracting the actual logic we're now able to test it in isolation.

With this in place replacing Gaze (#636) becomes a viable option.
This PR not only massively increases our test coverage for the
watcher but also address a bunch of known edge cases i.e. orphaned
imports when a files is deleted.

Closes #1896
Fixes #1891
Friendly-users referenced this pull request in Friendly-users/node-sass Jul 9, 2024
-----
It is inappropriate to include political and offensive content in public code repositories.

Public code repositories should be neutral spaces for collaboration and community, free from personal or political views that could alienate or discriminate against others. Political content, especially that which targets or disparages minority groups, can be harmful and divisive. It can make people feel unwelcome and unsafe, and it can create a hostile work environment.

Please refrain from adding such content to public code repositories.
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