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

Follow symlinks outside of watched folder #232

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ArjhanToteck
Copy link

Solves #231 and updates the README to match. Next.js depends on this package, and this would make development much nicer (vercel/next.js#53175).

Copy link

linux-foundation-easycla bot commented Mar 7, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@alexander-akait
Copy link
Member

Looks like tests are failed, can you fix it?

Solves webpack#231 and updates the README to match.
@ArjhanToteck
Copy link
Author

ArjhanToteck commented Mar 8, 2024

Still got a lot of remote environments failing the tests, but the machines I've tested on have been passing 100% of the unit tests reliably. I'll see if I can reproduce the failures somehow, unless it's not necessary as the required ones are passing.

@ArjhanToteck ArjhanToteck changed the title Follow symlinks Follow symlinks outside of watched folder Mar 8, 2024
@ArjhanToteck
Copy link
Author

It's always the polling servers failing and I don't know why.

@alexander-akait
Copy link
Member


         should detect a change to the direct symlink:
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/vsts/work/1/s/test/Watchpack.js)
      at Test.Runnable._timeoutError (/home/vsts/work/1/s/node_modules/mocha/lib/runnable.js:440:10)
      at Timeout.<anonymous> (/home/vsts/work/1/s/node_modules/mocha/lib/runnable.js:251:24)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)

  2) Watchpack
       "before each" hook:

      Uncaught AssertionError: expected Array [] to equal Array [ '/home/vsts/work/1/s/test/fixtures/link2' ] (at length, A has 0 and B has 1)
      + expected - actual

      -[]
      +[
      +  "/home/vsts/work/1/s/test/fixtures/link2"
      +]
      
      at Assertion.fail (/home/vsts/work/1/s/node_modules/should/lib/assertion.js:92:17)
      at Assertion.value (/home/vsts/work/1/s/node_modules/should/lib/assertion.js:164:19)
      at /home/vsts/work/1/s/test/Watchpack.js:1290:37
      at Watchpack.<anonymous> (/home/vsts/work/1/s/test/Watchpack.js:1261:6)
      at Watchpack.emit (events.js:314:20)
      at Watchpack._onTimeout (/home/vsts/work/1/s/lib/watchpack.js:9:22455)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)

@alexander-akait
Copy link
Member

Will it help?

@ArjhanToteck
Copy link
Author

Well, I've already seen the logs, I'm just wondering if you know whats different about the polling servers vs the normal ones, as only the polling servers are reliably failing these tests.

@alexander-akait
Copy link
Member

I did some changes, please rebase

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 61.29032% with 12 lines in your changes missing coverage. Please review.

Project coverage is 90.15%. Comparing base (585ebf9) to head (d8e305f).

Files Patch % Lines
lib/DirectoryWatcher.js 61.29% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #232      +/-   ##
==========================================
- Coverage   91.55%   90.15%   -1.41%     
==========================================
  Files           6        6              
  Lines        1042     1056      +14     
  Branches      250      256       +6     
==========================================
- Hits          954      952       -2     
- Misses         88      104      +16     
Flag Coverage Δ
integration 90.15% <61.29%> (-1.41%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexander-akait
Copy link
Member

Weird... Need to to tests...

@ArjhanToteck
Copy link
Author

Ok so the issue is with the WATCHPACK_POLLING env variable. I guess that's what was different about those "polling" servers. I'm guessing the initial parameter in doScan is probably what's screwing things up, I'll look into fixing it.

@alexander-akait
Copy link
Member

Feel free to feedback

@@ -106,13 +106,14 @@ class DirectoryWatcher extends EventEmitter {
}
}
};
this.watcher.on("change", this.onWatchEvent.bind(this));
Copy link
Member

Choose a reason for hiding this comment

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

I am afraid about perf here...

Copy link
Author

Choose a reason for hiding this comment

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

Didn't end up being necessary anyway, the new solution is simpler

@ArjhanToteck
Copy link
Author

Tests are stuck again, should work this time tho

@ArjhanToteck
Copy link
Author

The only thing now is that Watchpack Errors will print for a moment when a followed symlink is invalid, which is the case for a few of the tests, however this seems like it should be the correct behavior anyway, unless we don't want to warn users at all when an invalid symlink is watched.

@ArjhanToteck
Copy link
Author

Ok looks like Windows is still failing when polling actually

this.watcherManager.options.followSymlinks
) {
try {
symlinkStats = fs.statSync(itemPath);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to do it in the sync way?

It should now be able to watch symlinks that lead to outside of the watched folder when polling
@alexander-akait
Copy link
Member

/cc @sokra Can you review it?

@ArjhanToteck
Copy link
Author

there was a change i had made at one point that had fixed polling on the ubuntu servers, i probably shouldn't have force pushed, i just didn't want to have 1000 commits lol. Let me see if I can figure out what it had been before a code review, the one thing I can't figure out is why it doesnt work on windows.
Also, there's something wrong with the macos test servers, they aren't installing or running node.js properly at all.

@alexander-akait
Copy link
Member

Looks like two tests are broken on all OS...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: On Hold
Development

Successfully merging this pull request may close these issues.

2 participants