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

Version 9 strips leading ./ #495

Closed
romainmenke opened this issue Feb 28, 2023 · 15 comments · Fixed by #500
Closed

Version 9 strips leading ./ #495

romainmenke opened this issue Feb 28, 2023 · 15 comments · Fixed by #500

Comments

@romainmenke
Copy link

In version 8 :

./gulp/assets/js/*.js

becomes

./gulp/assets/js/app.js

In version 9 :

./gulp/assets/js/*.js

becomes

gulp/assets/js/app.js

Is this intentional?
I didn't find anything about it in the Changelog.

To me this seems like a bug because find does it like version 8

;find ./gulp/assets/js/*.js
./gulp/assets/js/app.js
@isaacs
Copy link
Owner

isaacs commented Feb 28, 2023

From the changelog:

  • Paths are returned in the canonical formatting for the platform in question.

  • Patterns ending in / will still be restricted to matching directories, but will not have a / appended in the results. In general, results will be in their default relative or absolute forms, without any extraneous / and . characters, unlike shell matches. (The mark option may still be used to always mark directory matches with a trailing / or .)

The new algorithm no longer has the ability to track the use of extraneous dot and slash characters, because each path matched is an object in a tree, and patterns are optimized aggressively. This allows for much faster searching (like, 5-500x faster in many cases), but there's no way to know with certainty which form of the pattern matched any given path.

@romainmenke
Copy link
Author

Paths are returned in the canonical formatting for the platform in question.

Can you give examples of this?
Is this the direct cause of what I am seeing?


Patterns ending in / will still be restricted to matching directories, but will not have a / appended in the results. In general, results will be in their default relative or absolute forms, without any extraneous / and . characters, unlike shell matches. (The mark option may still be used to always mark directory matches with a trailing / or .)

This section seems focussed on path endings, not path prefixes.

@romainmenke
Copy link
Author

romainmenke commented Feb 28, 2023

This allows for much faster searching (like, 5-500x faster in many cases), but there's no way to know with certainty which form of the pattern matched any given path.

I never thought this package was slow, I did however like that it just worked :/

Webpack for example just doesn't work when the leading ./ is omitted.

@msev
Copy link

msev commented Feb 28, 2023

hi @romainmenke

In my webpack configuration i needed to change that :

glob.sync('./dev/scripts/*.js')

by this :

globSync(path.resolve(process.cwd(), './dev/scripts/*.js'))

@romainmenke
Copy link
Author

@msev Thank you :)
Good to know that others are facing the same issue in the same context!

I however know how to fix it but I first wanted to know if this was a bug or not.

@geekact
Copy link

geekact commented Feb 28, 2023

globSync(path.resolve(process.cwd(), './dev/scripts/*.js'))

@msev maybe this can be globSync(path.resolve('./dev/scripts/*.js')) or globSync(path.join(process.cwd(), './dev/scripts/*.js'))

@isaacs
Copy link
Owner

isaacs commented Feb 28, 2023

If you want to get the absolute paths, you can do globSync('dev/scripts/*.js', { absolute: true }). Using path.resolve() in on the pattern in this way will break on Windows, because the pattern will contain \ characters.

@msev
Copy link

msev commented Feb 28, 2023

thanks @isaacs

@isaacs
Copy link
Owner

isaacs commented Feb 28, 2023

Ah, sorry, @romainmenke, I missed your other message:

Paths are returned in the canonical formatting for the platform in question.

Can you give examples of this? Is this the direct cause of what I am seeing?

So, in a nutshell, this means that the returned paths are the "normal" relative paths (or absolute paths if { absolute: true } is used) for the platform it's running on. So if you do .////foo/./../bar/*.js, then the returned paths on posix platforms will be like bar/baz.js rather than .////foo/./../bar/baz.js. On Windows, they'll be bar\\baz.js, using the platform-specific path separator.

The reason for this is that it's not tracking the full string that it walked down to find the match, it's just doing a walk over the file system and hanging onto the parts of the pattern that are still relevant at any given point. In other words, it's doing a file search, not a string expansion. Doing it in this way is radically more efficient, and makes a lot of weird edge cases easier to handle, plus it makes deduplication tremendously easier, and allows for much more efficient handling of .. and ** within patterns, but it means I dropping the information about extra slashes and dots in the pattern.

Patterns ending in / will still be restricted to matching directories, but will not have a / appended in the results. In general, results will be in their default relative or absolute forms, without any extraneous / and . characters, unlike shell matches. (The mark option may still be used to always mark directory matches with a trailing / or .)

This section seems focussed on path endings, not path prefixes.

This part in the middle:

In general, results will be in their default relative or absolute forms, without any extraneous / and . characters, unlike shell matches.

@romainmenke
Copy link
Author

The reason for this is that it's not tracking the full string that it walked down to find the match, it's just doing a walk over the file system and hanging onto the parts of the pattern that are still relevant at any given point. In other words, it's doing a file search, not a string expansion. Doing it in this way is radically more efficient, and makes a lot of weird edge cases easier to handle, plus it makes deduplication tremendously easier, and allows for much more efficient handling of .. and ** within patterns, but it means I dropping the information about extra slashes and dots in the pattern.

@isaacs That's fair.

The issue is also relatively simple to work around.

That others are facing the exact same issue seems to indicate that the ecosystem depends on the old behavior.

Webpack for example could handle foo/bar.js and ./foo/bar.js in the same way, but it doesn't, likely because it didn't need to.

Arguably it might have been better to publish such an extensive rewrite as a new package.
At the same time other packages should not ossify around incorrect assumptions (e.g. relative paths start with ./)


Maybe good to call out this case in the changelog with an example?
I would not have bothered to create an issue if I had known this was intended behavior.

@isaacs
Copy link
Owner

isaacs commented Feb 28, 2023

Yeah, as a maintainer, the thing about a community ossifying around your old code is that it's really hard to know when going into a massive overhaul like this exactly what can be discarded and what needs to be preserved, so long-coming much-needed semver major upgrades usually involve a bit of this back and forth. No matter how you try to get the word out there, sometimes the only real way to measure the importance of a feature is by ripping it out, lol

I can see a couple of ways forward (not mutually exclusive).

  1. Add a dotRelative option (spelling tbd) which would tell glob to prepend ./ onto any relative path it returns. (I'd push this to be an option in PathScurry, as well.)
  2. Yes, I agree this should be called out more clearly in the changelog. It's sort of implicitly there, but clearly not as obvious as it needs to be, I agree.
  3. I could track whether a pattern starts with ./, and imply dotRelative like I imply absolute for absolute patterns. But, doing this would entail kind of a lot of extra work and be fairly kludgey. The relative/absolute bitflag would need to be a trinary, minimatch would have to change its pattern optimization, etc. So I'm not very inclined to go that way.
  4. Don't add support for it, be intransigent that '' is a perfectly valid relative path, ./foo/bar is the same as foo/bar, etc. That's not really my style, unless there's some compelling reason not to add a given option, but it's worth mentioning. Webpack really shouldn't be treating them differently, but it is what it is.

@isaacs
Copy link
Owner

isaacs commented Feb 28, 2023

Oh, another question, if the result starts with ../ then does it still need the ./ ahead of it? In other words, is ./../foo/bar required, or is ../foo/bar ok with webpack or other tools that require a dot ahead of relative paths?

I'm assuming that the differentiation here stems from the fact that require('foo/bar') is meaningfully different from require('./foo/bar'), in which case ../ would be just as good as ./ for establishing relative-ness.

isaacs added a commit that referenced this issue Feb 28, 2023
@isaacs
Copy link
Owner

isaacs commented Feb 28, 2023

@romainmenke Care to try out #500 and see if it solves your issue?

npm i github:isaacs/node-glob#pull/500

And then:

glob(myPattern, { dotRelative: true })
// or whatever function you're using to get glob results

isaacs added a commit that referenced this issue Feb 28, 2023
@romainmenke
Copy link
Author

romainmenke commented Feb 28, 2023

No matter how you try to get the word out there, sometimes the only real way to measure the importance of a feature is by ripping it out, lol

I agree, and just did the same some weeks ago in postcss-preset-env, we are still dealing with the fallout 😄


Add a dotRelative option (spelling tbd) which would tell glob to prepend ./ onto any relative path it returns. (I'd push this to be an option in PathScurry, as well.)

Care to try out #500 and see if it solves your issue?

Running CI now, no errors have surfaced yet, so this is looking good!
(it takes a while for it to finish fully and report if there are any output diffs)

Edit : finished without issues 🎉

Thank you already for the super speedy action on this.

Yes, I agree this should be called out more clearly in the changelog. It's sort of implicitly there, but clearly not as obvious as it needs to be, I agree.

🙇

I could track whether a pattern starts with ./, and imply dotRelative like I imply absolute for absolute patterns. But, doing this would entail kind of a lot of extra work and be fairly kludgey. The relative/absolute bitflag would need to be a trinary, minimatch would have to change its pattern optimization, etc. So I'm not very inclined to go that way.

Yeah that sounds less than ideal.

An option at least adds some friction on the users side.
That will make it less likely that people will set it to true and depend upon that behavior. (Hopefully eliminating the issue over time.)

Don't add support for it, be intransigent that '' is a perfectly valid relative path, ./foo/bar is the same as foo/bar, etc. That's not really my style, unless there's some compelling reason not to add a given option, but it's worth mentioning. Webpack really shouldn't be treating them differently, but it is what it is.

That is also fine with me :)

My main concern at first was knowing if this was a bug or not and trying to understand what changed and why.


Oh, another question, if the result starts with ../ then does it still need the ./ ahead of it? In other words, is ./../foo/bar required, or is ../foo/bar ok with webpack or other tools that require a dot ahead of relative paths?

It doesn't seem to require ./../

I tried :

./../js/*.js

becomes :

../js/app.js

Webpack ran without issues.

@romainmenke
Copy link
Author

This is working perfectly on our end.
We only set the flag for lists of files going to Webpack to minimize our dependence on it.

Thank you for the fast updates here!

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 a pull request may close this issue.

4 participants