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

Add option to change followSymlinks #72

Conversation

immortal-tofu
Copy link

There is many projects who need to configure this option. This simple PR allow to pass a followSymlinks options to change it. Default is still false.

Webpack issue:
webpack/webpack#1866

Watchpack issue:
#61

@jsf-clabot
Copy link

jsf-clabot commented Mar 16, 2018

CLA assistant check
All committers have signed the CLA.

@@ -51,7 +51,7 @@ function DirectoryWatcher(directoryPath, options) {
this.watcher = chokidar.watch(directoryPath, {
ignoreInitial: true,
persistent: true,
followSymlinks: false,
followSymlinks: !!options.followSymlinks,
Copy link
Member

Choose a reason for hiding this comment

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

Breaking change, default is not false, default is boolean value options.followSymlinks

Copy link
Author

Choose a reason for hiding this comment

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

If you don't specify options.followSymlinks, it will be undefined. Well, we'll have !!options.followSymlinks === !!undefined === false. So yes, default is still false, no breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

@birdy- hm, you are right

@codecov
Copy link

codecov bot commented Mar 16, 2018

Codecov Report

Merging #72 into master will decrease coverage by 0.28%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
- Coverage    95.5%   95.22%   -0.29%     
==========================================
  Files           3        3              
  Lines         356      356              
  Branches       97       97              
==========================================
- Hits          340      339       -1     
- Misses         16       17       +1
Impacted Files Coverage Δ
lib/DirectoryWatcher.js 95% <ø> (-0.42%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d839e8...22cda93. Read the comment docs.

@RomainGiordano
Copy link

Let's ship it

@adrianodimarco
Copy link

Yes, ship it!

@shunxing
Copy link

Go ahead

@alexander-akait
Copy link
Member

alexander-akait commented Mar 16, 2018

I sent link on PR in closed webpack slack channel, just wait 👍

Copy link
Member

@ooflorent ooflorent left a comment

Choose a reason for hiding this comment

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

This change looks fine. However, how would you set this option in webpack? Could you post an example here?

@immortal-tofu
Copy link
Author

immortal-tofu commented Mar 16, 2018

I think it would be next to symlink: false|true in resolve section, but tbh it can be discussed.

resolve: {
  ...
  symlink: false,
  followSymlinks: true,
},

@ooflorent
Copy link
Member

ooflorent commented Mar 16, 2018

Then you should also submit a PR to update the schema if this one lands 👍

@ooflorent
Copy link
Member

I'm tempted to accept the PR but I'm worried about having symlink and followSymlinks options. This is not very intuitive for newcomers...

@immortal-tofu
Copy link
Author

I'll open a PR, but I'll have to update package.json's dependencies as well since it will be mandatory to update watchpack.
You can merge this part, and we'll discuss about the name in webpack on the future PR.

@TheLarkInn
Copy link
Member

@ooflorent I definitely agree with you. Can we open up an issue slated for v5 so we can change the default to be what the user also expects out of the box.

@immortal-tofu
Copy link
Author

Added an issue on webpack webpack/webpack#6845

@oliverguenther
Copy link

Anything we can do to help this move forward? When using followSymlinks with either npm link or, e.g., Angular CLI, this is a serious issue that I would hope to use in v4 already. Overriding the flag manually does work but is a very brittle workaround.

@Globegitter
Copy link

Globegitter commented Aug 11, 2018

Yep, can this please get merged? Seems there are quite a few people who would benefit from this change and merging it here is the first step necessary to make it possible to add it to webpack - and also just because it is merged here does not mean that webpack has to expose the option yet.

Also following semver this is a backwards compatible change and should ideally be shipped with v4.

@joeybaker
Copy link

joeybaker commented Sep 7, 2018

@birdy- maybe you could rebase this branch? It would be great to get this merged!

@immortal-tofu
Copy link
Author

I rebased. Ready to ship it captain'

@joeybaker
Copy link

@ooflorent @evilebottnawi @TheLarkInn Would it be possible to get another look at this PR? It would be a really helpful bug fix.

@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #72 into master will decrease coverage by 0.28%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
- Coverage    95.5%   95.22%   -0.29%     
==========================================
  Files           3        3              
  Lines         356      356              
  Branches       97       97              
==========================================
- Hits          340      339       -1     
- Misses         16       17       +1
Impacted Files Coverage Δ
lib/DirectoryWatcher.js 95% <ø> (-0.42%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d839e8...22cda93. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #72 into master will decrease coverage by 0.28%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
- Coverage    95.5%   95.22%   -0.29%     
==========================================
  Files           3        3              
  Lines         356      356              
  Branches       97       97              
==========================================
- Hits          340      339       -1     
- Misses         16       17       +1
Impacted Files Coverage Δ
lib/DirectoryWatcher.js 95% <ø> (-0.42%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d839e8...22cda93. Read the comment docs.

@joeybaker
Copy link

Is the drop in code coverage a blocker on getting this merged?

@Madorakkusu
Copy link

Can we ship it ?

@abhafedh
Copy link

we need that pleaaaaaase :( ship it

@DeTeam
Copy link

DeTeam commented Oct 24, 2018

Once this lands, we can remove a couple of linux-specific hacks in our projects, symlink-related.

@ingdir
Copy link

ingdir commented Oct 24, 2018

We have a really dirty hack in our system for a linux-based project, that one can be removed if this gets merged. Would be great to ship it.

@kostspielig
Copy link

This is much needed for us! To be merged soon? 👍

@sibelius
Copy link

this would be cool

@mko
Copy link

mko commented Nov 8, 2018

This is needed for our project as well. It would be really useful to have this released ASAP.

@DeTeam
Copy link

DeTeam commented Dec 14, 2018

@ooflorent any chance to have a look at this PR? ^_^

@bfutterleib
Copy link

+1 love to have this, free hugs?

@Globegitter
Copy link

@sokra what is the story now with watchpack 2? It seems you implement your own watcher now. Does this support a followSymlink option?

@dhonx
Copy link

dhonx commented Jun 14, 2019

I also got the same problem ... thank you everyone ... I think the webpack team would be very good if adding this feature (follow Symlink option)

@sokra
Copy link
Member

sokra commented Jul 10, 2019

#114 adds the followSymlink option

@sokra sokra closed this Jul 10, 2019
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