Added cache support, removed extra 'fs' calls for performance, etc.

dcombslinkedin commented 5 years ago

Most code now in index.js.

Left code in lib that is still used externally (e.g. deps-for, resolve-pkg, pkg) that is no longer needed for hash-for-dep itself.

stefanpenner commented 5 years ago

@dcombslinkedin nice, but it looks like there are some new failing tests, mind fixing?

dcombslinkedin commented 5 years ago

Thanks, Stef. As I haven't found a way to run checks for lint or es5 against the module (is there one?), it's been a one-at-a-time parade of fixes. Working on the latest update now.

stefanpenner commented 5 years ago

stefanpenner closed this 20 seconds ago

Hmm, i have no memory of this...

stefanpenner commented 5 years ago

@dcombslinkedin broadly this looks good, I left some nit-picks and touch ups. Do to the size of the change, I'll need to spend some extra time with it yet, I hope to have my review complete by EOD.

stefanpenner commented 5 years ago

I've restarted the windows tests.

dcombslinkedin commented 5 years ago

Thanks, Stef. I'm heading up to Redmond today for a MS conf on 1ES/1DS and some other build stuff tomorrow. I'll try to get changes in tonight after I arrive, so I should have something for you either tonight or Friday morning.

stefanpenner commented 5 years ago

@dcombslinkedin I've fixed my above nit-picks, and forced push.

stefanpenner commented 5 years ago

I am refactoring/extracting and writing tests, so we can have confidence in the various algorithms

stefanpenner commented 5 years ago

I've made the following changes:

split the functions and algorithms into there own files for testing/reading
caches are now passed as arguments to functions
the outermost function hashForDep now selects between the global cache or a function allocated local cache. This removes the need to manage the global state, instead allowing the GC to handle it all for us.

stefanpenner commented 5 years ago

@dcombslinkedin after implementing the missing tests, can you investigate the windows failures? I have confirmed prior to your additions, the windows tests were not hanging. I have also ensured they are green on master: https://ci.appveyor.com/project/embercli/hash-for-dep/builds/22008926

dcombslinkedin commented 5 years ago

I'm making a few changes in addition to addressing the items above. New changes include:

Cleaned up unused imports from index.js
Converted 'genNameDirKey' and 'genRealPathKey' with direct references to cacheKey, and added a comment to cacheKey. It seems overkill to have separate files and test files for what are basically one-line wrappers around the cacheKey calls. I'm happy to add tests to the existing cache-key tests if that's necessary/useful. As part of this, removed the gen-name-dir-key.js and tests/gen-name-dir-key-test.js.

stefanpenner commented 5 years ago

FYI: https://github.com/stefanpenner/hash-for-dep/pull/48

stefanpenner commented 5 years ago

looks like windows is green!

dcombslinkedin commented 5 years ago

Yeah, the change to force core.symlinks to true seems to have done it. I'll remove the debug stuff and probably add a couple more tests and push one more time for your (hopefully last) review.

dcombslinkedin commented 5 years ago

Ignore this if it passes - forgot an 'it.only' and to squash things. Doing that now.

dcombslinkedin commented 5 years ago

I'm going to try to squash, but have been having issues with it. Wish me luck!

stefanpenner commented 5 years ago

released as v1.4.0 🎉

stefanpenner / hash-for-dep

Added cache support, removed extra 'fs' calls for performance, etc. #45