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 dependency: glob@8.0.3 #202

Closed
wants to merge 1 commit into from
Closed

Conversation

strager
Copy link
Contributor

@strager strager commented Oct 23, 2022

glob 7.x depends on the path-is-absolute package. glob 8.x does not. Switching Jasmine to glob 8.x reduces npm package bloat.

The following changes in this commit are visible to Jasmine users:

  • \ is now only used as an escape character, and never as a
    path separator in glob patterns, so that Windows users have a
    way to match against filenames containing literal glob pattern
    characters.
  • Glob pattern paths must use forward-slashes as path
    separators, since \ is an escape character to match literal
    glob pattern characters.

https://github.com/isaacs/node-glob/blob/v8.0.3/changelog.md#80

glob 7.x depends on the path-is-absolute package. glob 8.x does not.
Switching Jasmine to glob 8.x reduces npm package bloat.

The following changes in this commit are visible to Jasmine users:

> * `\` is now **only** used as an escape character, and never as a
>   path separator in glob patterns, so that Windows users have a
>   way to match against filenames containing literal glob pattern
>   characters.
> * Glob pattern paths **must** use forward-slashes as path
>   separators, since `\` is an escape character to match literal
>   glob pattern characters.

https://github.com/isaacs/node-glob/blob/v8.0.3/changelog.md#80
@strager
Copy link
Contributor Author

strager commented Oct 23, 2022

I have not tested this change on Windows. I don't know if this change is desirable for Jasmine from a usability point of view.

@sgravrock
Copy link
Member

Thanks for the PR. Unfortunately this completely breaks Jasmine on Windows. (CI would've told you that, but it looks like Circle was broken when you submitted your PR.) We use path.join to prepend parent directories to glob expressions, and it emits backslashes. The result is that no glob expression can match anything.

Beyond that, it's a fairly nasty breaking change for Windows users: if somebody uses backslashes in e.g. a spec_files glob, it'll silently stop matching anything. In some cases that would cause the test suite to pass even though not all of the intended files run. Making such a change isn't completely out of the question, but it'd have to be done in a major release and preceded by a prominent deprecation warning.

Probably the best way to do this is to wait for node-glob#468 and for Jasmine's dev dependencies to upgrade to glob 8.x. Then we should be able to configure glob so that it's not a breaking change. If that doesn't pan out, I'd prefer to hold off on upgrading glob as long as possible, while also printing a deprecation warning if user-supplied globs contain backslashes.

@sgravrock sgravrock closed this Oct 23, 2022
@sgravrock
Copy link
Member

sgravrock commented Oct 23, 2022

See also #1970.

@strager
Copy link
Contributor Author

strager commented Oct 23, 2022

Probably the best way to do this is to wait for isaacs/node-glob#468 and for Jasmine's dev dependencies to upgrade to glob 8.x. Then we should be able to configure glob so that it's not a breaking change.

That sounds like a great idea. 👍

@sgravrock
Copy link
Member

Now that I understand what's going on with glob a bit better, I think it makes sense to adopt Glob 8's default behavior. Jasmine is commonly used in cross-platform situations such as software that's developed on both Windows and other OSes, or teams that use Windows locally but Linux for CI. In those environments, having backslash mean different things on different OSes is a footgun that's best removed.

The Glob 8 upgrade is on the 5.0 branch and will ship with that release.

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.

None yet

2 participants