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

Proposal to refactor how transactions are processed #787

Closed
wants to merge 0 commits into from

Conversation

jessyao26
Copy link

In response to issue #740
Implementation
I went through /web-auth/index.js and saw duplicate param arrays [ 'redirectUri', 'responseType', 'scope', 'audience', '_csrf', 'state', '_intstate', 'nonce' ]
Some param arrays were passing in slightly different strings for different prototype methods (i.e. responseMode). I created a 'base' param array that gets concated with custom values at the top of the index.js file.
More ideal implementation
Ideally, I would create a file of constants separate from this index.js file and require it whereever needed. This file of constants could help fill in vars with vscode's intellisense and eliminate magic strings, but I didn't want to make any large changes.
Notes
-There's two unrelated lines that have different spacing. I didn't touch these lines. I'm unsure if it's a prettifier or minifier that's adding spacing.
-I tried to use the 'const' keyword instead of vars, when setting arrays, but I think I'm pointing to an older version of es6
-There's another similar params array inside /web-auth/popup.js, but it has different params, if there was a constants file (from the more ideal implementation section) this would be a use case
Feedback
let me know if there's anything that can be improved! Or if you're passing on this, feel free to let me know why!

@luisrudge
Copy link
Contributor

Thanks @jessyao26 for your PR! I think we should go with your ideal implementation here, because we actually use this kind of baseOptions merge in a a lot of places, so abstracting away this specific code makes a lot of sense.

@jessyao26
Copy link
Author

see #792 !

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.

2 participants