search

jasmine / jasmine-npm

A jasmine runner for node projects.
MIT License
376 stars 145 forks source link

Upgrade dependency: glob@8.0.3 #202

Closed strager closed 1 year ago

strager commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

See also #1970.

strager commented 1 year ago

Probably the best way to do this is to wait for https://github.com/isaacs/node-glob/issues/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. :+1:

sgravrock commented 1 year ago

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.