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

Document breaking changes for glob@8 #467

Closed
s100 opened this issue Apr 12, 2022 · 25 comments · Fixed by strapi/strapi#15140
Closed

Document breaking changes for glob@8 #467

s100 opened this issue Apr 12, 2022 · 25 comments · Fixed by strapi/strapi#15140

Comments

@s100
Copy link

s100 commented Apr 12, 2022

It looks like glob@8 has been released, congratulations! We would love to upgrade, but before that, would it be possible to document the changes, particularly the breaking changes, for this release, in the change log? Thank you in advance.

@j0k3r
Copy link

j0k3r commented Apr 12, 2022

Looks like it's only Node >= 12
3d6c4cd

@sircharlo
Copy link

sircharlo commented Apr 12, 2022

Looks like it's only Node >= 12 3d6c4cd

More than that, I believe... v7.2.1...v8.0.1

@XhmikosR
Copy link

@isaacs
Copy link
Owner

isaacs commented Apr 12, 2022

The minimatch 5 breaking change is not a breaking change for glob, because it's always coerced all slashes to /. It's only a breaking change if you're using minimatch directly. (And if you're relying on glob's minimatch behaving a particular way, and don't have a dependency on it yourself, well... that's not wise.)

But sure, I'll add a changelog.

@isaacs isaacs closed this as completed in 7eab927 Apr 12, 2022
@sircharlo
Copy link

Weird, my project completely broke with version 8. I must have misunderstood something.

@ath0mas
Copy link

ath0mas commented Apr 13, 2022

I also had to change my patterns sent to glob when updating from v7.2 to v8.0, to not get empty array instead of correct list results.

On Windows, and mixing or joining path values with "child" glob patterns, it works quite fine directly with v7.2, while with v8.0 you are required to replace backslashes with forward-slashes (basic replace, or like using normalize-path or unixify packages).

  • does it come from the minimatch v3 to v5 update? (or v3.0 to v3.1 in unpublished 7.2.1 yet?)
    if so, it should be added to the changelog.md I think
  • even if the README mentions the "backslashes vs. forward-slashes" it in its Windows section, maybe it should be more explicit about how to manipulate path values and meet the expected glob pattern "format" and/or the "tools" to help do so. add a warning in the changelog telling it may have worked before but it is not the case anymore, requiring to review and possibly update your patterns.

Sorry for the ref, but fast-glob helped me understand with its

Use the normalize-path or the unixify package to convert Windows-style path to a Unix-style path.

@sircharlo
Copy link

sircharlo commented Apr 13, 2022

I'm trying upath as we speak to see if that would work in my case. For sure a heads up would've been nice, as it worked before and is broken now for "\\" (read: non-UNIX) style paths.

@sircharlo
Copy link

I'm trying upath as we speak to see if that would work in my case. For sure a heads up would've been nice, as it worked before and is broken now for "\" (read: non-UNIX) style paths.

Confirmed, using upath as a drop-in replacement for path works great along with glob.

@larshp
Copy link

larshp commented Apr 13, 2022

Also breaks stuff for me, probably due to windows paths as noted above.

@isaacs
Copy link
Owner

isaacs commented Apr 13, 2022

This caveat has been in the readme for a a decade now (added in 386f7e8). But if someone has a repro case showing something that worked in v7 and doesn't work in v8, I'd be happy to take a look.

@larshp
Copy link

larshp commented Apr 13, 2022

thanks

suggest adding a note about the caveat in the changelog, as the behavior changes from 7.2.0 to 8.0.0? My issue occurs in https://github.com/abaplint/abaplint/blob/main/packages/cli/src/file_operations.ts, however I have not taken the caveat into account, and also not debugged it, so its probably an issue in my library.

@domoritz
Copy link

The update broke widows support for me. Here is a repro case: vega/ts-json-schema-generator#1221.

@icebob
Copy link

icebob commented Apr 19, 2022

Same issue here. On Windows, version 8.x.x doesn't find files in folder that works on 7.2.0

@juergba
Copy link

juergba commented Apr 24, 2022

Upgrade to v8.0.1 breaks Mocha as well.

@nicolas377
Copy link

Using backslashes was never explicitly supported by glob, see the windows portion of the README. My current solution is to resolve paths and then normalize them to forward slashes. See this example:

// assuming the pattern is stored in string variable pattern, and that node's path module has been imported
const normalizedPattern = path.resolve(pattern).split(path.sep).join("/");
glob.sync(normalizedPattern);

The workaround will most likely change soon, assuming the implementation of #468 lands.

@JustinBeckwith
Copy link

Not to pile on, but this broke linkinator as well:
JustinBeckwith/linkinator#389

Very specifically it changed something with paths on windows. I'll dig in and try to get a more minimal repo.

@isaacs
Copy link
Owner

isaacs commented May 6, 2022

Just to repeat what was already said above:

  • This is a change that occurred in a semver major. It should not have broken anyone unexpectedly, unless you are depending on * or >= or something foolish like that.
  • It is dropping support for a use case that has not been supported, explicitly in the readme, for 10 years.
  • It fixed the use of escape characters on Windows platforms (which was broken before).

A v8 minor will come with the ability to explicitly choose to use \ as a path separator and not an escape character in #468.

Since cwd and root are always paths, and not patterns, we can add /-coercion to those automatically.

@JustinBeckwith
Copy link

Apologies, I used "broke" a tad flippantly :) The PR to update the dependency caused CI to fail in a way that wasn't obvious in the changelog. Thanks for the detailed explanations!

@isaacs
Copy link
Owner

isaacs commented May 6, 2022

@JustinBeckwith No worries! I definitely knew what you meant. More just trying to forestall the next dozen comments complaining about glob breaking their programs 😅

@SimenB
Copy link

SimenB commented May 6, 2022

More just trying to forestall the next dozen comments complaining about glob breaking their programs

I'd just like to say that's the most relatable comment I've ever seen on GitHub! 😅

Also - I appreciate the work you've done for the community, node and npm through the years. A major version breaking assumptions is not a huge issue.

Thank you! ♥️

@s100
Copy link
Author

s100 commented May 9, 2022

  • This is a change that occurred in a semver major. It should not have broken anyone unexpectedly, unless you are depending on * or >= or something foolish like that.
  • It is dropping support for a use case that has not been supported, explicitly in the readme, for 10 years.
  • It fixed the use of escape characters on Windows platforms (which was broken before).

Would it be possible to document these changes in the change log?

@ronenteva
Copy link

Just to repeat what was already said above:

  • This is a change that occurred in a semver major. It should not have broken anyone unexpectedly, unless you are depending on * or >= or something foolish like that.
  • It is dropping support for a use case that has not been supported, explicitly in the readme, for 10 years.
  • It fixed the use of escape characters on Windows platforms (which was broken before).

A v8 minor will come with the ability to explicitly choose to use \ as a path separator and not an escape character in #468.

Since cwd and root are always paths, and not patterns, we can add /-coercion to those automatically.

You are 100% right, but why not mention it in the changelog?

@swrdfish
Copy link

Same issue came up on Windows for version 7.2.2 also. Was working fine with 7.2.0

@nicolas377
Copy link

please upgrade to 7.2.3

@swrdfish
Copy link

@nicolas377 thanks! That worked.

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