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

fix(cache): Switch to lru-cache to save ourselves from unlimited memory consumption #38

Closed
wants to merge 1 commit into from

Conversation

iarna
Copy link
Contributor

@iarna iarna commented Jul 10, 2018

No description provided.

@coveralls
Copy link

coveralls commented Jul 10, 2018

Coverage Status

Coverage increased (+0.03%) to 95.238% when pulling 4ebabd3 on lru-cache into ab35a90 on latest.

@isaacs isaacs closed this in e518222 Aug 4, 2019
@ljharb
Copy link

ljharb commented Aug 11, 2019

@isaacs heads up that this was a breaking change for node < 4 in a non-major version. There's no "engines" declaration and you didn't stop testing on node 4 until after this was landed.

I'm using yargs 7 → read-pkg-up 1.0.1 → read-pkg 1.1.0 → normalize-package-data 2.5.0 → hosted-git-info 2.8. I'm not using a lockfile because it's in a package, not an app, and it'd be really appreciated if 2.8+ could be reverted, and republished as v3. (another alternative would perhaps be to use an older version of lru-cache that didn’t use node 4+ syntax)

ljharb added a commit to ljharb/safe-publish-latest that referenced this pull request Aug 11, 2019
@isaacs
Copy link
Contributor

isaacs commented Aug 12, 2019

Sure, I can do that.

But seriously. @ljharb. You gotta have a frank talk with whoever you're supporting who uses node < 4. It's not safe. They're putting their systems at risk. Whatever the cost to upgrade, it surely is less than the cost of getting pwned, and if they're doing new builds with new OSS code, this problem will only get worse.

isaacs added a commit that referenced this pull request Aug 12, 2019
…ted memory consumption"

This reverts commit e518222.

#38 (comment)

Will un-revert in semver-major bump.
@ljharb
Copy link

ljharb commented Aug 12, 2019

Thanks, appreciate it.

@ljharb
Copy link

ljharb commented Aug 12, 2019

@isaacs hmm, it looks like the use of const here (added in a5a91ac) is causing the same issue. Could that be converted to var for a v2.8.4 release?

@isaacs
Copy link
Contributor

isaacs commented Aug 12, 2019

@ljharb Done.

Have you had a sit-down with whoever is running your code on node 0.x yet? It's irresponsible to let them continue doing this.

@ljharb
Copy link

ljharb commented Aug 13, 2019

Thanks!

It’s not necessarily irresponsible. There are many uses of node, and many tech stacks, that make the insecurities of old versions irrelevant. (Additionally, although not for this package, old node is a great standin for old browsers, so on anything that could run in browsers, it’s very useful to maintain support)

isaacs added a commit to npm/npm-package-arg that referenced this pull request Nov 11, 2019
BREAKING CHANGE: this drops support for ancient node versions.
See npm/hosted-git-info#38 (comment)
isaacs added a commit to npm/npm-pick-manifest that referenced this pull request Nov 11, 2019
BREAKING CHANGE: this drops support for ancient node versions.
See npm/hosted-git-info#38 (comment)
@wraithgar wraithgar deleted the lru-cache branch September 21, 2021 14:23
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.

4 participants