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

Remove dependency on util.promisify #5478

Closed
alfaproject opened this issue Jul 14, 2021 · 3 comments
Closed

Remove dependency on util.promisify #5478

alfaproject opened this issue Jul 14, 2021 · 3 comments
Labels
size/small Estimated to take LESS THAN A DAY

Comments

@alfaproject
Copy link
Contributor

Given that we only support newer versions of node, I don't believe this is still needed?

require('util.promisify').shim();

@glasser
Copy link
Member

glasser commented Jul 15, 2021

@abernix Thoughts? @alfaproject is correct that this shim isn't needed on any version of Node we currently support in AS3.

On the other hand, I think we vaguely attempt to support non-Node environments for some of our code?

On the other other hand, it looks like promisify is only used in apollo-server-cache-memcached. So if we remove this, we only break people trying to use apollo-server-cache-memcached in non-Node environments, and in that case they can just use the util.promisify shim themselves?

@glasser glasser added this to the MM-2021-07 milestone Jul 15, 2021
@glasser glasser added the size/small Estimated to take LESS THAN A DAY label Jul 16, 2021
@alfaproject
Copy link
Contributor Author

Yes, I don't think it should be in core because that shim imports a few other dependencies. apollo-server-cache-memcached could even just do a simple promise wrapper instead of needing that utility method anyway. It's 3 lines of code maximum.

@abernix
Copy link
Member

abernix commented Jul 19, 2021

@glasser I'm quite supportive of removing it in lieu of either some treatment within apollo-server-cache-memcached or just removal and put the burden on non-Node.js environment users (they typically require a bundler, anyhow). It's been a while since I took this on, but I'd even claim that the continued inclusion is an oversight on my part in the upgrade to support only newer Node.js versions. Perhaps our Node.js minimum version changed between the time that that initiative was taken on and when we actually released.

Overall, I'd be somewhat surprised if non-Node.js + Memcached was a popularized cohort. I'd vote for removing it and I think we should get this into a 3.x bugfix ASAP.

glasser added a commit that referenced this issue Jul 20, 2021
This was required for compatibility with Node 6, but we now only support
Node 12+.

It appears to only be used by apollo-server-cache-memcached.

Fixes #5478.
glasser added a commit that referenced this issue Jul 20, 2021
Node 6 support meant we needed to polyfill util.promisify,
Object.values, and Object.entries. But we now only support Node 12+.

Fixes #5478.
@hwillson hwillson removed this from the MM-2021-07 milestone Jul 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size/small Estimated to take LESS THAN A DAY
Projects
None yet
Development

No branches or pull requests

4 participants