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

Upgrade glob to 7.1.2 #45

Merged
merged 1 commit into from
Jul 5, 2017
Merged

Upgrade glob to 7.1.2 #45

merged 1 commit into from
Jul 5, 2017

Conversation

pkuczynski
Copy link
Contributor

@kevva
Copy link
Contributor

kevva commented Jul 3, 2017

Not needed since ^7.0.3 will use 7.x.x. Read: nodesource.com/blog/semver-tilde-and-caret for more info.

@kevva kevva closed this Jul 3, 2017
@pkuczynski
Copy link
Contributor Author

The thing is that the minimum version where it works is 7.1.x. @zinserjan can you please comment on this?

@zinserjan
Copy link

Not needed since ^7.0.3 will use 7.x.x.

Yep, and that's the problem as we rely on the absolute option of 7.1, but ^7.0.3 matches also 7.0.x versions.
There are scenaroius were you'll get 7.0 instead of 7.1 when you npm install, either when npm 7.0 is the most common version and npm hoist it to the root or when the version is already in the cache. I had some of these issue in the past and it's hard to find the reason for this, when you use a feature that is not fully covered by the version range.

In my opinion there are 3 ways to fix this:

  1. remove See the node-glob options. from readme and document every supported option (of course only 7.0 features)
  2. bump the glob version to latest (this makes the readme in sync again + makes sure that all of these features are usable)
  3. move glob to peerDependencies so that the user can choose the version

I would prefer the 2nd option as it's the easiest, does not break other libraries (peerDependency may would) and it's safe to bump as there are no breaking changes in 7.1.

@pkuczynski
Copy link
Contributor Author

@sindresorhus what's your opinion on this?

@pkuczynski
Copy link
Contributor Author

@kevva could you please re-consider this PR?

@sindresorhus sindresorhus reopened this Jul 5, 2017
@sindresorhus sindresorhus merged commit 962148e into sindresorhus:master Jul 5, 2017
@sindresorhus
Copy link
Owner

Alright, but we're not doing a new release until #46 is merged, so you might want to add a dependency on glob in your project in the meantime, so it's deduped to the correct version.

@pkuczynski
Copy link
Contributor Author

Got it. Fingers crossed it would happen soon. mocha-webpack do have glob as a dependency (see https://github.com/zinserjan/mocha-webpack/pull/145/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2).

mocha-webpack is actually not my problem, but I am trying to solve an issue with the latest version, which breaks running tests from WebStrom IDE, which is using absolute paths.

@pkuczynski pkuczynski deleted the patch-1 branch July 5, 2017 17:05
kevva pushed a commit that referenced this pull request Jul 24, 2017
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.

4 participants