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

Update globby requirement from ^9.2.0 to ^10.0.1 #4254

Merged
merged 24 commits into from Jan 3, 2020

Conversation

dependabot-preview[bot]
Copy link
Contributor

@dependabot-preview dependabot-preview bot commented Sep 5, 2019

Updates the requirements on globby to permit the latest version.

Release notes

Sourced from globby's releases.

v10.0.1

  • Don't throw when specifying a non-existing cwd directory (#125) a226f5d

sindresorhus/globby@v10.0.0...v10.0.1

Commits

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Dependabot will not automatically merge this PR because it includes an out-of-range update to a production dependency.


Note: This repo was added to Dependabot recently, so you'll receive a maximum of 5 PRs for your first few update runs. Once an update run creates fewer than 5 PRs we'll remove that limit.

You can always request more updates by clicking Bump now in your Dependabot dashboard.

Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
  • @dependabot use these labels will set the current labels as the default for future PRs for this repo and language
  • @dependabot use these reviewers will set the current reviewers as the default for future PRs for this repo and language
  • @dependabot use these assignees will set the current assignees as the default for future PRs for this repo and language
  • @dependabot use this milestone will set the current milestone as the default for future PRs for this repo and language
  • @dependabot badge me will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot dashboard:

  • Update frequency (including time of day and day of week)
  • Automerge options (never/patch/minor, and dev/runtime dependencies)
  • Pull request limits (per update run and/or open at any time)
  • Out-of-range updates (receive only lockfile updates, if desired)
  • Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

@hudochenkov
Copy link
Member

Appveyor didn't pick up this PR. Will try to close/open to trigger it.

@hudochenkov hudochenkov closed this Sep 5, 2019
@dependabot-preview
Copy link
Contributor Author

OK, I won't notify you again about this release, but will get in touch when a new version is available.

If you change your mind, just re-open this PR and I'll resolve any conflicts on it.

@hudochenkov hudochenkov reopened this Sep 5, 2019
Updates the requirements on [globby](https://github.com/sindresorhus/globby) to permit the latest version.
- [Release notes](https://github.com/sindresorhus/globby/releases)
- [Commits](sindresorhus/globby@v9.2.0...v10.0.1)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
@dependabot-preview dependabot-preview bot force-pushed the dependabot/npm_and_yarn/globby-tw-10.0.1 branch from 24e4fc2 to e1442b4 Compare September 5, 2019 19:32
@vankop
Copy link
Member

vankop commented Sep 9, 2019

Looks like fix works. Problem was related to fast-glob@3 https://github.com/mrmlnc/fast-glob/releases/tag/3.0.0

Only forward-slashes in glob expression.
 Previously, we convert all slashes to the forward-slashes,
which did not allow the use of escaping. See pattern syntax section in the README.md file.

So we have to convert all our tests cases to forward-slashes (mostly stuff with __dirname or cmd)

Maybe it will be required to note this for our users as well

@vankop
Copy link
Member

vankop commented Sep 9, 2019

I think we need create discussion about this, since my implementation is not perfect. There is no way to pass \ in file name (it will be converted to forward-slash, I don't know actually is in *nix it allowed), no ability to use backslashes for escaping characters. On the other hand, if we just update tests it will require all users to use replace(/\\/, '/')

if (typeof fileList === "string") {
fileList = [fileList];
}
let fileList = [].concat(files).map(file => file.replace(/\\/g, "/"));
Copy link
Member

Choose a reason for hiding this comment

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

It can potentially output linux paths on windows, we need test 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 did not understand what you exactly mean =(
If you mean that stylelint can try to output fixed code to wrong (linux-like) file path, than we have such tests, see https://github.com/stylelint/stylelint/blob/master/system-tests/fix/fix.test.js#L60

Copy link
Member

Choose a reason for hiding this comment

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

So I found only one workaround for

There is no way to pass \ in file name

and

no ability to use backslashes for escaping characters

allow users escape with double backslashes only 🙀

@alexander-akait
Copy link
Member

Because we prepare new major release it can be breaking change like:

Breaking
You can no longer pass in glob pattern with backward-slashes, as backward-slashes have special meaning with globbing. You can still pass in normal non-glob paths with backward-slashes.

And only fix our tests

@vankop
Copy link
Member

vankop commented Sep 10, 2019

Because we prepare new major release it can be breaking change like:

Breaking
You can no longer pass in glob pattern with backward-slashes, as backward-slashes have special meaning with globbing. You can still pass in normal non-glob paths with backward-slashes.

And only fix our tests

you can pass it, but it will be interpreted as escaping next character

I agree with your suggestion to update only tests. However, stylelint will become platform specific (especially it is a problem for cli run)

@alexander-akait
Copy link
Member

alexander-akait commented Sep 10, 2019

@vankop no, many glob implementations don't support \ and fast-glob (new glob system in globby) do same, it is expected. glob should always use /, but it can be some painfull for people who use something like

stylelint .\path\to\file.csss

Maybe we should look in eslint code base and find what they do in this situation

@alexander-akait
Copy link
Member

alexander-akait commented Sep 10, 2019

Problem converting \ to / is what some files can use \ in filename (*nix) and you break them

@vankop
Copy link
Member

vankop commented Sep 10, 2019

Problem converting \ to / is what some files can use \ in filename (*nix) and you break them

only if they have double backslashes =) anyway solution with updating tests is better, I just pointed that it will require some changes to our users

I will take a look on eslint solution, too.

@alexander-akait
Copy link
Member

Also windows support / as path separator

@hudochenkov
Copy link
Member

Maybe we should update globby just yet. Because we might need whole new approach (borrowed from ESLint :)) to handle inputs and fix problems like #4211, #4193, #3273, #3272.

@vankop
Copy link
Member

vankop commented Sep 10, 2019

Actually eslint uses minimatch and do all stuff around (glob matching) itself and looks like it assumes that users provide glob pattern with forward-slashes https://github.com/eslint/eslint/blob/94e39d9f782f45db86a079e07508d63040118ef1/Makefile.js#L1098

@alexander-akait
Copy link
Member

I think we can do same

@vankop
Copy link
Member

vankop commented Sep 10, 2019

Ok 🤝

So.. to sum up ✍🏼:

  1. we reverting this and fix tests instead
  2. notifying users about correct glob pattern 🚨(in next major release)

@vankop vankop mentioned this pull request Nov 7, 2019
6 tasks
@ybiquitous ybiquitous mentioned this pull request Nov 12, 2019
@hudochenkov
Copy link
Member

We're going to release new major in beginning of January. @vankop do you thing you make required changes till then?

@vankop
Copy link
Member

vankop commented Dec 28, 2019

I think we need just to resolve conflicts

@vankop
Copy link
Member

vankop commented Jan 2, 2020

@hudochenkov I think we need write a note (replaceBackslashes required for correct path output) about writing tests with fixtures

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Good job!

@hudochenkov
Copy link
Member

I need help with changelog entry. If understand correctly, this might be a breaking change for some users. Here's what I understand:

  • Changed: globby was updated to v10. Now only forward-slashes (/) should be used as directory separator in globs. Refer to glob pattern syntax. Most of the users wouldn't need to change anything, but Windows users might need to update their globs. (#4254).

@ntwb
Copy link
Member

ntwb commented Jan 3, 2020

I think that's a reasonable assumption that its a breaking change for Windows users

@hudochenkov hudochenkov merged commit 59957f0 into master Jan 3, 2020
@hudochenkov hudochenkov deleted the dependabot/npm_and_yarn/globby-tw-10.0.1 branch January 3, 2020 14:36
@hudochenkov
Copy link
Member

  • Changed: globby was updated to v10. Now only forward-slashes (/) should be used as directory separator in globs. Refer to glob pattern syntax. Most of the users wouldn't need to change anything, but Windows users might need to update their globs. (#4254).

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

Successfully merging this pull request may close these issues.

None yet

4 participants