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.

Sign up for GitHub

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

Jump to bottom

Separate crypto_utils generateRandom implementation for browser & Node #38

Merged
merged 5 commits into from
Nov 20, 2017
Merged

Separate crypto_utils generateRandom implementation for browser & Node #38

merged 5 commits into from
Nov 20, 2017

Conversation

Meligy
Copy link
Contributor

@Meligy Meligy commented Nov 13, 2017 •
edited
Loading

Also moves TypeScript dependencies to devDependencies

Fixes #35
Fixes #37
Fixes #39

Updates the library version to 0.3.0

@Meligy
Copy link
Contributor Author

Meligy commented Nov 14, 2017

@tikurahul would be great if you can let me know what you think once you get a chance.

Cheers,

@juriwiens
Copy link

@Meligy I would prefer to not move all of the TypeScript type definitions to devDependencies, because they are useful when used within another TypeScript project (see #22)

@Meligy
Copy link
Contributor Author

Meligy commented Nov 14, 2017

It's very conventional to put @types/* in devDependencies. Is there any other example that you are aware of that advices against this?

jQuery is a special case. You could argue the library shouldn't even rely on it. That would be a separate issue in itself that should not be addressed here. Other libraries used like hapi or request are not exposed as arguments anyway.

@Meligy Meligy mentioned this pull request Nov 14, 2017
Closed
@Meligy
Copy link
Contributor Author

Meligy commented Nov 14, 2017

The current behaviour with latest TypeScript is that jQuery settings shows as any, and unless you go to the actual .d.ts file you don't get any errors (maybe thanks to existing compiled files).

However, I'll revert jQuery especially to keep the full intellisense, so it can be tackled as its own thing.

@Meligy
Keep jQuery in dependencies not devDependencies
d4ff37a 
This ensures we still have typing for xor jQuerySettings
And removes any doubt about reopening openid#22
@Meligy
Copy link
Contributor Author

Meligy commented Nov 14, 2017

Done.

@tikurahul
Copy link
Collaborator

@Meligy Thanks for the PRs. I am swamped with a few other things today. I will get back to as soon as I can. Thanks for your patience.

@juriwiens
Copy link

@Meligy I'm also of the opinion that @types/* should be put in dependencies in general. But those who are declaring types which are getting exposed, should be put in dependencies IMHO. Because when I import a library, I want it to have all it's dependencies included, so it's just "ready to use".

And you're right, jQuery seems to be the only one whose types are reexported. I didn't notice that 😅

@juriwiens juriwiens mentioned this pull request Nov 16, 2017
Closed
@tikurahul
Copy link
Collaborator

jQuery request types are also used in xhr so there is a bit of cleanup we need to do to move off of jQuery. I want to move to fetch but support on older Android devices is poor. So I chose jQuery and made it pluggable.

I will review this PR today. I only have a few nit picks. I am excited about an official React Native sample. Maybe you can publish it for others and we can link it off the official repo?

tikurahul
tikurahul reviewed Nov 16, 2017
View reviewed changes
src/authorization_request_handler.ts Outdated
@@ -79,11 +72,15 @@ export const BUILT_IN_PARAMETERS = ['redirect_uri', 'client_id', 'response_type'
* using various methods (iframe / popup / different process etc.).
*/
export abstract class AuthorizationRequestHandler {
constructor(public utils: QueryStringUtils) {}
constructor(public utils: QueryStringUtils, generateRandom?: RandomGenerator) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could rewrite this as:

constructor(public utils: QueryStringUtils, generateRandom: RandomGenerator = cryptoGenerateRandom) {

src/crypto_utils.ts Outdated

export function generateRandom(sizeInBytes: number = DEFAULT_SIZE) {
let buffer: Uint8Array = new Uint8Array(sizeInBytes);
export const generateRandom: RandomGenerator = (sizeInBytes = DEFAULT_SIZE) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call this method cryptoGenerateRandom so we don't have to alias this.

tsconfig.json
@@ -8,6 +8,7 @@
],
"rootDir": "src",
"outDir": "built",
"noImplicitAny": true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

I can't believe I did not have this until now :)

@Meligy
Copy link
Contributor Author

Meligy commented Nov 16, 2017

@tikurahul I did all the required refactoring now.

Also applied default param refactoring to similar constructor params.

@Meligy
Copy link
Contributor Author

Meligy commented Nov 17, 2017 •
edited
Loading

React Native sample might come, but I'm myself very new to React Native (I run an Angular meetup in Sydney, LOL!) and was only able to work this because I joined an already created project (standing on the shoulders of giants). Maybe in a week or 2 when I manage to feel more comfortable around this.

@tikurahul tikurahul merged commit 1332495 into openid:master Nov 20, 2017
@tikurahul
Copy link
Collaborator

Published v0.3.0 for the library. Thanks for the PR @Meligy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tikurahul tikurahul tikurahul left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Meligy @juriwiens @tikurahul