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 tests with node 15 #228

Merged
merged 5 commits into from
Feb 23, 2021
Merged

Fix tests with node 15 #228

merged 5 commits into from
Feb 23, 2021

Conversation

Chocobozzz
Copy link
Contributor

Please after the merge, could you create a patch release?

It blocks node upgrade of projects that uses your library: Chocobozzz/PeerTube#2700

Thanks for your work :)

@coveralls
Copy link

coveralls commented Nov 12, 2020

Coverage Status

Coverage remained the same at 97.531% when pulling b3d30ec on Chocobozzz:hotfix/node-15 into f27cb2b on kwhitley:master.

@rimiti
Copy link

rimiti commented Nov 23, 2020

@kwhitley Is it possible to merge this PR?

@Chocobozzz Chocobozzz mentioned this pull request Dec 8, 2020
@kontrollanten
Copy link
Contributor

kontrollanten commented Dec 8, 2020

@Chocobozzz Maybe we should remove node 6 support since the CI tests are broken there? I think that all support for node <9 should be removed, but I guess that's out of scope for this PR :)

@rimiti
Copy link

rimiti commented Dec 8, 2020

@kontrollanten Agreed with you, node v6 could be serenely removed.

Like some other users (I guess) I'd like to use the latest Node LTS version (v14), is it possible for you @kwhitley to push -up this urgent topic?

kwhitley and others added 2 commits December 8, 2020 11:22
Co-authored-by: kontrollanten <[email protected]>
Co-authored-by: kontrollanten <[email protected]>
- 10
- 11
- 12
- 14
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@kontrollanten
Copy link
Contributor

As far as I understand the failing tests is due to a race condition in clearing cache cancels expiration callback, both trace backs are pointing to that tests. But I've to insight in how apicache works to understand what's supposed to be tested; by the name it looks like it's testing that the expiration callback isn't called, but by the code it looks like something else. Someone with more knowledge are welcome to help :)

@Chocobozzz
Copy link
Contributor Author

I fixed the expiration tests

@kwhitley
Copy link
Owner

kwhitley commented Dec 9, 2020

Thanks for the fix @Chocobozzz! I got to the failing point yesterday and had to shelve it again...

I'm going to release this (sadly) as a major version, as it absolutely breaks backwards compat (by dropping old Node versions). The only question is whether to even try to maintain restify support within this new scope. If no one uses it, better to drop support along with the old Node versions to simplify the codebase and make future support less painful.

Thoughts?

@kwhitley
Copy link
Owner

kwhitley commented Dec 9, 2020

Getting this error with the branch:
image

Then under restify as well:
image

image

@rotsee
Copy link

rotsee commented Dec 11, 2020

The only question is whether to even try to maintain restify support within this new scope.

FWIW I use this with Restify in production, and would hate to see that support dropped... :/

@Chocobozzz
Copy link
Contributor Author

@kwhitley I don't have these errors, and neither does travis. What is your nodejs version?

This PR was just fixing tests and add node 15 support, not removing node 6 support. I think we should just merge the test fixes, support node 15 and keep compat' with restify and node 6. They worked in 1.5.2 so I don't see why they would not work anymore now.

@rimiti
Copy link

rimiti commented Dec 11, 2020

Agreed with what you said @Chocobozzz.

@Chocobozzz
Copy link
Contributor Author

What do you think @kwhitley?

Copy link

@rimiti rimiti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vikasg603
Copy link

Any update for the new release?
I am using Node 14, and I am continuously getting the warning from npm.

@kwhitley kwhitley merged commit 7ffacfe into kwhitley:master Feb 23, 2021
@kwhitley
Copy link
Owner

Merged and going out tonight... thanks so much @Chocobozzz and everyone else for your patience! <3

Sincerest apologies for how ridiculously long this has taken to push through... I've been swamped with a few other libraries and my upcoming Cloudflare Workers work (on top of full time job)!

Cheers all and thanks again!

@Chocobozzz
Copy link
Contributor Author

No problem thanks for you work and your lib @kwhitley 😽

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.

7 participants