replace lru-cache dependency

[mbtools]

This change removes the dependency on the lru-cache package. It is replaced by a comparable, but much simpler class in this package. The cache is still set at max 1000 entries and works the same way as before.

Performance looks to be very similar. However, it needs a realistic test case that pushes the limits of the cache. I will leave it to more experts to judge.

References

#447, #448
#695
npm/cli#7350

[wraithgar]

Normally we try not to ask too much of contributors in the way of commits, and this is one of the reasons why we typically don't accept dependency change PRs. For those we'd prefer if the removal of lru-cache was a discrete deps: remove lru-cache commit so that it shows up in the changelog.

If it's not too much to ask can you make that package.json change a single commit w/ an appropriate commit message? Everything else you can keep in a second commit with a fix: prefix.

[wraithgar]

Our PR linter will pass if the PR subject or every commit has a valid conventional commit message. I've purposely made the PR subject a non-conventional one to reflect the fact that we intend to rebase-merge this PR with the deps and fix commits.

[wraithgar]

Sorry for the confusion on the commit messages. If this is more than you want to do go ahead and get it all in here however you want and then we can get this PR green and we'll branch off of it and get the commits right.

[mbtools]

Back to fully working. I will let you create a new PR with two commits as required. Thx!

[wraithgar]

Thank you! We'll let this sit for a bit for the community to weigh in, and we need some time to properly review it ourselves (we are in the middle of a large refactor in npm itself).

[lukekarrys]

I'm in favor of this PR and converting our lru-cache to an internal dependency.

If we do land this, I think we should remove any functionality that we aren't currently using in semver. Looking at the code that is only cache.set() and cache.get() (and set will call delete()). All my specific comments are about removing things. This is just my opinion and will let others weigh in first. So no need for anyone to make all these changes immediately 😄

[wraithgar]

I'm in favor of the same @lukekarrys. I would like to see us get to a point where we're not testing semver's internal cache directly, but only as a side effect. That is, only testing it exactly how it's used during normal semver operations. Unit testing the module itself isn't that useful, all it does is show we HAVE a working cache, not that it's used by semver properly.

[wraithgar]

@lukekarrys the title of this PR is intentionally not conventional so that we remember to rebase it into two commits: one for the deps change and one for the feat

[mbtools]

Sounds good. We can certainly reduce the cache functions to a minimum. I just had them in there to prove to myself that it's working like lru-cache.

If we're convinced the cache is fine, I suggest removing entries(), capacity(), the snapshots, and the max type check. But keep has() and size() so that the basic operations test still completes and we have 100% coverage.

The cache is transparent to the outside of range parse. Showing that semver uses it properly, is / should be visible only using runtime measurements. Not sure how you prefer to implement those.

[wraithgar]

The cache is transparent to the outside of range parse. Showing that semver uses it properly, is / should be visible only using runtime measurements. Not sure how you prefer to implement those.

Here's a proof of concept showing how we could assure that subsequent new Range instances get back the same range set.

const Semver = require('.')
const cached = Symbol('cached')
const r1 = new Semver.Range('1.0.0')
r1.set[0][cached] = true
const r2 = new Semver.Range('1.0.0')
console.log(r1.set[0][cached]) // Will be true
console.log(r2.set[0][cached]) // Will be true, showing it's cached.

We can test this all inside a t.test('cache') inside the range class tests.

[mbtools]

• Removed unnecessary cache methods & tests
• Kept basic cache test to ensure 100% coverage
• Added a test to range class as suggested

[wraithgar]

Kept basic cache test to ensure 100% coverage

This still runs the risk that we have code in there we never use and don't need. If we aren't using the code from range we don't need to test it.

I think the cache size is the only thing left right? Can we loop through 1001 versions to test that the first one dropped out of the cache? Can the max size be hard set in lru-cache for now? There's no need to pass it in if we never set it to anything else.

[lukekarrys]

@wraithgar We still use a coverage map in this repo. I'm think that will require testing the cache directly to get full coverage, right?

[wraithgar]

We can edit that file.

[wraithgar]

Here's an example of how we did it in npm itself when we wanted to test a component of all the commands in a new test file:

https://github.com/npm/cli/blob/21b823edf7b3095ce1f53da7befda2b41296afe4/test/coverage-map.js

[wraithgar]

I think that we should fix the tests but also not block this PR on "perfect" code. I'm good w/ this as-is (pending an actual review of the code at this point). We can clean it up on our own time.

[wraithgar]

We have not forgotten about this PR. The big npm display layer refactor is almost done and then we can come back to things like this PR.

[wraithgar]

#704