Remove fast-glob dependency

MicahZoltu commented 4 years ago

Jasmine currently has two dependencies, jasmine-core and fast-glob. jasmine-core has no transitive dependencies, but fast-glob has ~93 transitive dependencies. I'm not sure what exactly fast-glob is being used for, but it certainly sounds like a library that is not a critical piece of Jasmine, and certainly not something that needs 93 dependencies to function.

Consider looking for alternative implementations to use, or inline whatever functionality it is you need from fast-glob. If it weren't for that one dependency, Jasmine would be an incredibly low dependency testing framework, which is exactly what I am looking for (and why I used to use Jasmine).

An significant iterative improvement here would be to just update fast-glob to latest, as it has greatly reduced its dependency set between version 2.2.7 and 3.2.4 (from 93 to 16).

Edit: Looks like my estimate was a bit off, in reality it appears to have 121 transitive dependencies, all from fast-glob:

npm install jasmine
npm WARN deprecated urix@0.1.0: Please see https://github.com/lydell/urix#deprecated
npm WARN deprecated resolve-url@0.2.1: https://github.com/lydell/resolve-url#deprecated
added 121 packages from 101 contributors and audited 146 packages in 3.215s

slackersoft commented 4 years ago

Jasmine uses file globbing to allow users to specify wildcards in their list of spec files to run. Previous to this release we used the glob package, but ended up picking up fast-glob as part of #153. I've updated the dependency to use the most recent fast-glob, but we won't likely make a new release just for this.

Thanks for reporting the issue on transitive dependencies. We try to keep Jasmine's dependency graph small, but sometimes things sneak in or there just isn't a good way around them.

apla commented 4 years ago

http://npm.broofa.com/?q=jasmine

slackersoft commented 4 years ago

I've updated the version of fast-glob for the next release, but that doesn't quite seem like enough of an impetus to release a 3.6.2 of Jasmine, since that's currently the only change since the 3.6.1 release.

Globbing files has been part of Jasmine for some time now, so we don't want to just remove it. I'd be happy to have a discussion on which library we can use to do the globbing (ideally something with support and enough users), or if the update to the more recent version of fast-glob is good enough.

vfcp commented 4 years ago

Since you opened the debate about the library, I'm curious why the switch from glob to fast-glob in #153 ?

Looking at the dependencies in some of my projects, I can see some other popular packages like node-sass, rollup, rimraf, node-gyp all use glob. Actually, according to npmjs, glob has 18851 dependents vs fast-glob's 1117.

While I totally agree that popularity doesn't mean better, I believe consistent glob behavior with other tools commonly used with jasmine would be more beneficial than a (potential) few milliseconds performance boost.

But then again, I have no data to compare between glob and fast-glob in a large project and decisions should be supported by facts, not opinions :)

MicahZoltu commented 4 years ago

Data for comparison: glob has 10 dependencies. fast-glob (latest) has 16 dependencies.

vfcp commented 4 years ago

Becoming aware of #153 , I've decided to give --worker-count a try. There seems to be a lot of issues with it. To be sure the issues were not related with my specs, I've cloned jasmine-npm repo and modified package.json test script from "test": "./node_modules/.bin/grunt && ./bin/jasmine.js" to "test": "./node_modules/.bin/grunt && ./bin/jasmine.js --worker-count=2"

vfcp@Valters-MacBook jasmine-npm % npm test         

> jasmine@3.6.1 test /Users/vfcp/Projects/jasmine-npm
> grunt && ./bin/jasmine.js --worker-count=2

Running "jshint:all" (jshint) task
>> 28 files lint free.

Running "specs" task
Randomized with seed 55099
Started
.................................................................................................................................

129 specs, 0 failures
Finished in 0.293 seconds
Randomized with seed 55099 (jasmine --random=true --seed=55099)

Done.
Randomized with seed 26796
Started
..............................................................................F.FFF.F.F.FF..FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF/Users/vfcp/Projects/jasmine-npm/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:4037
          throw error;
          ^

TypeError: Cannot read property 'spies' of undefined
    at currentSpies (/Users/vfcp/Projects/jasmine-npm/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1840:56)
    at SpyRegistry.clearSpies (/Users/vfcp/Projects/jasmine-npm/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:8130:19)
    at clearResourcesForRunnable (/Users/vfcp/Projects/jasmine-npm/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1387:19)
    at QueueRunner.onComplete (/Users/vfcp/Projects/jasmine-npm/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1733:13)
    at Immediate.<anonymous> (/Users/vfcp/Projects/jasmine-npm/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:7120:12)
    at processImmediate (internal/timers.js:458:21) {
  jasmineMessage: "Uncaught exception: TypeError: Cannot read property 'spies' of undefined"
}
FFFFFFFFFFFFF./Users/vfcp/Projects/jasmine-npm/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:4037
          throw error;
          ^

TypeError: Cannot read property 'spies' of undefined
    at currentSpies (/Users/vfcp/Projects/jasmine-npm/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1840:56)
    at SpyRegistry.clearSpies (/Users/vfcp/Projects/jasmine-npm/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:8130:19)
    at clearResourcesForRunnable (/Users/vfcp/Projects/jasmine-npm/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1387:19)
    at QueueRunner.onComplete (/Users/vfcp/Projects/jasmine-npm/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1733:13)
    at Immediate.<anonymous> (/Users/vfcp/Projects/jasmine-npm/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:7120:12)
    at processImmediate (internal/timers.js:458:21) {
  jasmineMessage: "Uncaught exception: TypeError: Cannot read property 'spies' of undefined"
}

Failures:
1) manager reporter callback 'specStarted' should forward the payload directly
  Message:
    Error: <spyOn> : fork() method does not exist
    Usage: spyOn(<object>, <methodName>)
  Stack:
    Error: <spyOn> : fork() method does not exist
    Usage: spyOn(<object>, <methodName>)
        at <Jasmine>
        at UserContext.<anonymous> (/Users/vfcp/Projects/jasmine-npm/spec/manager_spec.js:10:5)
        at <Jasmine>
  Message:
    TypeError: cluster.fork is not a function
  Stack:
    TypeError: cluster.fork is not a function
        at exports (/Users/vfcp/Projects/jasmine-npm/lib/manager.js:29:13)
        at UserContext.<anonymous> (/Users/vfcp/Projects/jasmine-npm/spec/manager_spec.js:58:7)
        at <Jasmine>
        at processImmediate (internal/timers.js:458:21)

2) manager reporter callback 'specDone' should forward the payload directly
  Message:
    Error: <spyOn> : fork() method does not exist
    Usage: spyOn(<object>, <methodName>)
  Stack:
    Error: <spyOn> : fork() method does not exist
    Usage: spyOn(<object>, <methodName>)
        at <Jasmine>
        at UserContext.<anonymous> (/Users/vfcp/Projects/jasmine-npm/spec/manager_spec.js:10:5)
        at <Jasmine>
  Message:
    TypeError: cluster.fork is not a function
  Stack:
    TypeError: cluster.fork is not a function
        at exports (/Users/vfcp/Projects/jasmine-npm/lib/manager.js:29:13)
        at UserContext.<anonymous> (/Users/vfcp/Projects/jasmine-npm/spec/manager_spec.js:71:7)
        at <Jasmine>
        at processImmediate (internal/timers.js:458:21)

... up to 92)

Did I misunderstood the --worker-count option?

apla commented 4 years ago

Data for comparison: glob has 10 dependencies. fast-glob (latest) has 16 dependencies.

And now it's much better, shaving off 76 deps. Surprisingly, micromatch@3 package have 81 deps itself despite it's name.

slackersoft commented 4 years ago

Honestly, I didn't look too deeply at the details of the switch from glob to fast-glob in the multi-threading PR, other than at some of the benchmarks. I don't have a strong opinion either way on which library Jasmine uses to do file globbing. The main argument I'm hearing in favor of switching back to glob is that any given project is more likely to already have that be a transitive dependency than fast-glob.

I'm inclined to leave this decision a bit up to a vote, since the community is ultimately who gets stuck with the larger dependency tree.

👍 - Switch back to glob 👎 - Stick with (updated) fast-glob

jasmine / jasmine-npm

Remove fast-glob dependency #162