encrypt-node initial commit #12
Conversation
There was a problem hiding this comment.
This is a fairly shallow review as I'm not a domain expert in crypto. But things in general here look sane; the code looks to be well-documented, readable, and of high quality. I've left a few comments below of some odd smells, but in general I think this is good. Part of the fun of open sourcing is letting your community help you identify issues as well, so if you're worried about some code style or organizational stuff don't fret too much.
|
|
||
| import { | ||
| serializeFactory, aadFactory, | ||
| MessageHeader // eslint-disable-line no-unused-vars |
There was a problem hiding this comment.
Disabling no-unused-vars for specific imports seems strange. Why not remove the import? Are there side-effects?
There was a problem hiding this comment.
These imports are for TypeScript and are not used from the eslint perspective...
You make a comment below about linters... More there
| } from '@aws-crypto/serialize' | ||
| // @ts-ignore | ||
| import { Transform as PortableTransform } from 'readable-stream' | ||
| import { Cipher } from 'crypto' // eslint-disable-line no-unused-vars |
There was a problem hiding this comment.
Extra commentary: Use of eslint with TypeScript is interesting. tslint may be a better fit here and you can configure it to not overlap with your code formatter (e.g. tslint-config-standard).
There was a problem hiding this comment.
I'm not as sure https://eslint.org/blog/2019/01/future-typescript-eslint . TypeScript has adopted ESLint as it's standard.
But I do not really care for the unused var comments. The reason I flatten the imports is so that I can add this comment to specific type only imports. I've had a few fat finger moments if I just comment out the whole import ;)
|
|
||
| export function getFramedEncryptStream (getCipher: GetCipher, messageHeader: MessageHeader, dispose: Function) { | ||
| let accumulatingFrame: AccumulatingFrame = { contentLength: 0, content: [], sequenceNumber: 1 } | ||
| let pathologicalDrain: Function|false = false |
There was a problem hiding this comment.
Having this type be Function | null or Function | undefined may be more semantically correct, based on how this is used. But false does work fine.
There was a problem hiding this comment.
I go back and forth on this. I agree that undefined is a little more idiomatic. I even do it that way in several places. But this value is going to be swapped back and forth. So I would prefer to not have
pathologicalDrain = undefined floating around.
Especially since undefined = () => {badStuff} is valid JS. :) . Who ever thought that was a good idea... sigh
There was a problem hiding this comment.
Peanut gallery/IANAJSDev: I like the more explicit/unambiguous Function|false
There was a problem hiding this comment.
Np, I copied over what I have from decrypt. And turn this into a function that does work or a noop...
comments, and a little clean up
| // @ts-ignore | ||
| import { pipeline } from 'readable-stream' | ||
|
|
||
| const fromUtf8 = (input: string) => Buffer.from(input) |
There was a problem hiding this comment.
On one hand the default encoding is utf8, the default is unlikey to change because that is a breaking change. On the other hand Buffer.from(input, 'utf8') is more explicit. I lean explicit.
There was a problem hiding this comment.
Very good point. Changing it everywhere.
There was a problem hiding this comment.
This looks like it has been basically blessed by your JS reviewers and looks sane to me from an SDK perspective, so I am going to go ahead and approve.
There's a couple of open discussions but they look conversational rather than blocking. I gave my 2 cents where I had it, for what it's worth.
|
|
||
| export function getFramedEncryptStream (getCipher: GetCipher, messageHeader: MessageHeader, dispose: Function) { | ||
| let accumulatingFrame: AccumulatingFrame = { contentLength: 0, content: [], sequenceNumber: 1 } | ||
| let pathologicalDrain: Function|false = false |
There was a problem hiding this comment.
Peanut gallery/IANAJSDev: I like the more explicit/unambiguous Function|false
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.