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

Document timeDifference startegyOption #199

Closed
gr2m opened this issue Oct 21, 2020 · 5 comments · Fixed by #200
Closed

Document timeDifference startegyOption #199

gr2m opened this issue Oct 21, 2020 · 5 comments · Fixed by #200
Assignees

Comments

@gr2m
Copy link
Contributor

gr2m commented Oct 21, 2020

follow up to #164. Looks like we implemented the option without documenting it.

@copperwall would you mind adding the documentation for the option?

@copperwall
Copy link
Member

copperwall commented Oct 22, 2020

Oh yeah definitely! I'll get on that

@copperwall
Copy link
Member

Hey @gr2m, I was looking into documenting this, but it looks like timeDifference lives in the state parameter to auth instead of options parameter that the user passes in. So it doesn't look like that option is available for the user to configure.

We could add it as an option and then initialize the state with the timeDifference from options, or leave it undocumented? What do you think?

@gr2m
Copy link
Contributor Author

gr2m commented Oct 22, 2020

hmm sorry I found timeDifference defined in StrategyOptions:

auth-app.js/src/types.ts

Lines 76 to 89 in 90bc639

export type StrategyOptions = {
id: number | string;
privateKey: string;
installationId?: number | string;
clientId?: string;
clientSecret?: string;
request?: OctokitTypes.RequestInterface;
cache?: Cache;
timeDifference?: number;
log?: {
warn: (message: string, additionalInfo?: object) => any;
[key: string]: any;
};
};

but it really should only be set on State instead. I just checked and everything still works if we do the change

diff --git a/src/types.ts b/src/types.ts
index 7093605..99ca9a3 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -81,7 +81,6 @@ export type StrategyOptions = {
   clientSecret?: string;
   request?: OctokitTypes.RequestInterface;
   cache?: Cache;
-  timeDifference?: number;
   log?: {
     warn: (message: string, additionalInfo?: object) => any;
     [key: string]: any;
@@ -125,6 +124,7 @@ export type State = StrategyOptions & {
   installationId?: number;
   request: OctokitTypes.RequestInterface;
   cache: Cache;
+  timeDifference?: number;
   log: {
     warn: (message: string, additionalInfo?: object) => any;
     [key: string]: any;

Would you like to send a PR for that?

@copperwall
Copy link
Member

Ah yeah, I see what you mean. Thanks for pointing that out! I'll make a PR right now.

@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 2.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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 a pull request may close this issue.

2 participants