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

Coordinated fix on Feb 12 broke Windows path handling between node-glob and minimatch #471

Closed
jsuddsjr opened this issue May 14, 2022 · 12 comments

Comments

@jsuddsjr
Copy link

jsuddsjr commented May 14, 2022

When using allowWindowsEscape=true in minimatch options, glob is forcing minimatch to treat the path as though it contained escapes rather than Windows path separators (backslashes). Notice the negation in the following statement, which would entirely fix the issue if it were allowed to run.

  if (!options.allowWindowsEscape && path.sep !== '/') {
    pattern = pattern.split(path.sep).join('/')
  }

Therefore, glob@7.2.2 combined with minimatch@^3.1.1 cannot locate files using patterns like:

E:\\Src\\repo\\packages\\test\\.storybook\\main{'.js','.jsx'}

Unfortunately, this is causing the latest version of Storybook to fail to locate configuration files in its default state in Windows. Error: No configuration files have been found in your configDir.

Please coordinate another fix.

@dkrnl
Copy link

dkrnl commented May 14, 2022

similar issue after npm update

WARNING in svg-spritemap-webpack-plugin
No source files match the patterns: 'source\img\svg-sprite\*.svg'

@nicolas377
Copy link

nicolas377 commented May 14, 2022

For right now, the only solution I can think of is to pin glob at 7.2.1, or 7.2.0 if 7.2.1 is the issue.

@nicolas377
Copy link

@jsuddsjr Is this not an issue on 7.2.1? If so, how about 7.2.0?

@nicolas377
Copy link

The issue seems to be at 73feafd. It will not show up on non-windows systems.

allowWindowsEscape is documented here, which indicates that the translation from \ to / will not occur. That is an undocumented breaking change for windows users. I will open a PR to revert the setting of allowWindowsEscape. For right now, pin glob to 7.2.0 with npm overrides, like this:

package.json

  "overrides": {
    "glob": "7.2.0"
  }

nicolas377 pushed a commit to nicolas377/node-glob that referenced this issue May 14, 2022
This is a fix to isaacs#471, by a partial revert of 73feafd.
See my comment on the issue (isaacs#471 (comment)) for more details.
@h1orz
Copy link

h1orz commented May 14, 2022

@jsuddsjr Is this not an issue on 7.2.1? If so, how about 7.2.0?

7.2.0 is work.

@nicolas377
Copy link

Could you try overriding glob to my PR and checking if it works?

package.json

  "overrides": {
    "glob": "github:nicolas377/node-glob#revert-allowWindowsEscape"
  }

@h1orz
Copy link

h1orz commented May 14, 2022

Could you try overriding glob to my PR and checking if it works?

package.json

  "overrides": {
    "glob": "github:nicolas377/node-glob#revert-allowWindowsEscape"
  }

It is work.

  # yarn
  "resolutions": {
    "glob": "github:nicolas377/node-glob#revert-allowWindowsEscape"
  }

@novrain
Copy link

novrain commented May 15, 2022

stylus/stylus#2680

@jsuddsjr
Copy link
Author

@jsuddsjr Is this not an issue on 7.2.1? If so, how about 7.2.0?

7.2.1 introduced the flag, 7.2.0 works as expected.

@isaacs
Copy link
Owner

isaacs commented May 15, 2022

Can you try using glob 7.2.3?

If that doesn't work, idk, might just have to roll back that change entirely, or do a different thing.

@nicolas377
Copy link

The only problem was setting that config option to true, the upgrade didn't break anything. 7.2.3 should be working. If it is, I'll go ahead and close my PR.

@h1orz
Copy link

h1orz commented May 15, 2022

Great, 7.2.3 is work.

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

No branches or pull requests

6 participants