Possible typo: JwksClient.getSigningKey()

[sjawhar]

Purpose

I think there is a typo the JwksClient library. JwksClient.getSigningKey() is not defined as a class method according to ES6, but rather as a class property. Besides being inconsistent, it's making it harder for me to stub the method for tests :) This is because getSigningKey is not defined on JwksClient.prototype, but rather this.getSigningKey gets defined as a function in the constructor.

[QuantumLove]

👍

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

[lbalmaceda]

Can you please evaluate @stevehobbsdev?

[stevehobbsdev]

Looks good at a glance but just in case, I cloned it to run the tests and they fail.

Turns out there's also a bit of logic in the constructor that overwrites what getSigningKey is and the main reason why getSigningKey was defined as a field function rather than a class method:

node-jwks-rsa/src/JwksClient.js

Lines 28 to 33 in f301e1d

	if (this.options.rateLimit) {
	this.getSigningKey = rateLimitSigningKey(this, options);
	}
	if (this.options.cache) {
	this.getSigningKey = cacheSigningKey(this, options);
	}

This whole mechanism is a bit weird and I would encourage it to be rewritten to improve understanding. I can dedicate some cycles to doing it this week.

[stevehobbsdev]

Needs changed. Changing getSigningKey to a method without considering the wrapping mechanism causes tests to fail. I believe this mechanism needs to change.

[davidpatrick]

@sjawhar because of the way the wrappers interact with getSigningKey, it will unfortunately not be as simple as turning it into a class property. Some more work will need to be done with how the rateLimit and cache wrappers interact with it. This work is welcome, but if you don't have the bandwidth for it we will close this PR until further notice.