-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: remove figgy-pudding #11
Conversation
index.js
Outdated
single: false, | ||
strict: false | ||
} | ||
return Object.assign(opts, { ...defaults, ...opts }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: 806e8c8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont' think you need to do both Object.assign and the spread. Just return {...defaults, ...opts}
is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this isn't a class, so convention would be to name it ssriOpts
rather than SsriOpts
. and defaults
can be defined outside, since it never changes.
So, it'd look like:
const defaults = { ... }
const ssriOpts = (opts = {}) => ({ ...defaults, ...opts })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using only the spread operator returns a new object that doesn't allow for options mutation. You seem to have added some tests a while ago that break if options can't be mutated after the stream starts. Is that the intended behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should integrity
be a key with default value null
or something? Figgy would have returned an object that had that key by default. In case down stream things need to reference that property?
index.js
Outdated
@@ -380,9 +380,10 @@ function checkData (data, sri, opts) { | |||
module.exports.checkStream = checkStream | |||
function checkStream (stream, sri, opts) { | |||
opts = SsriOpts(opts) | |||
const checker = integrityStream(opts.concat({ | |||
const checker = integrityStream({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's actually safe to just set opts.integrity = sri
since ssriOpts
creates a clone of the object.
079a464
to
488fc0a
Compare
488fc0a
to
0e78fd7
Compare
This allows us to start a stream, then get the integrity value mid-way through, and THEN update the options with the expected integrity.
- Update devDeps - Run lint after tests, not before - Push to github before publishing, not after - Use GitHub Actions for CI instead of appveyor/travis - Remove outdated CoC, PR template, etc.
Removes
figgy-pudding
.See: https://github.com/npm/rfcs/blob/isaacs/internal-de-figgy-pudding/accepted/0000-npm-option-handling.md