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

ioredis plugin breaks ioredis #713

Closed
dyladan opened this issue Jan 21, 2020 · 5 comments · Fixed by #714
Closed

ioredis plugin breaks ioredis #713

dyladan opened this issue Jan 21, 2020 · 5 comments · Fixed by #714
Labels
bug Something isn't working

Comments

@dyladan
Copy link
Member

dyladan commented Jan 21, 2020

At the end of the constructor, the ioredis plugin returns this._moduleExports.prototype;. The types don't complain, but the ioredis module is completely broken and unuseable.

fix: return return this._moduleExports;

@dyladan dyladan added the bug Something isn't working label Jan 21, 2020
dyladan added a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Jan 21, 2020
@naseemkullah
Copy link
Member

Thanks! Why is it that tests passed? I copy pasted the return from another plugin I believe

@dyladan
Copy link
Member Author

dyladan commented Jan 21, 2020

The tests do not use the plugin loader, so they are independent from this behavior.

None of the other plugins have this behavior from a cursory check.

@naseemkullah
Copy link
Member

I think I was inspired by

this._moduleExports.RedisClient.prototype,
without quite understanding.

Anyhow, you think it's possible/worth it to include plugin loader in tests?

@dyladan
Copy link
Member Author

dyladan commented Jan 21, 2020

I can't think of an easy way to do that, and I don't want the test to depend on an outside component. In this case it was unfortunate that the module exports prototype happened to have the same type as the module exports. In most cases that won't be true. I'm not sure what would be a good general way to test this.

@naseemkullah
Copy link
Member

Sounds good, thanks.

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants