Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Exposing pkey, cert from crypto context to js land #4070

Closed
wants to merge 1 commit into from

Conversation

ssuda
Copy link

@ssuda ssuda commented Oct 1, 2012

This will solve exposing pkey, cert from pkcs12/pfx to js
issue #4050

@indutny
Copy link
Member

indutny commented Oct 1, 2012

Why not just .getKey() and .getCert()? Also your patch is very unclean (commented lines, etc).

@bnoordhuis
Copy link
Member

@ssuda Can you fix the style issues first?

@ssuda
Copy link
Author

ssuda commented Oct 1, 2012

Styling issues were fixed

@ssuda
Copy link
Author

ssuda commented Oct 7, 2012

@bnoordhuis please review this



void SecureContext::SetCert(Local<String> property, Local<Value> value,
const AccessorInfo& info) {
Copy link
Member

Choose a reason for hiding this comment

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

Style: if arguments don't fit on a single line, make them line up, one per line.

@bnoordhuis
Copy link
Member

Some style issues but the basic concept seems sound. I agree with @indutny though, it should use functions instead of a getter/setter.

This will solve exposing pkey, cert from pkcs12/pfx to js
@ssuda
Copy link
Author

ssuda commented Oct 10, 2012

@bnoordhuis, style issues were fixed. To make them enumerable I am going with properties rather than Get/Set functions

@bnoordhuis
Copy link
Member

To make them enumerable I am going with properties rather than Get/Set functions

That's very much unlike anything else in the crypto API.

@isaacs
Copy link

isaacs commented Feb 4, 2013

I agree with @bnoordhuis and @indutny here. No magic getter/setter accessors. We do those sorts of shenanigans as a last resort only. Put that in a userland JS module if you'd prefer that style.

@Nodejs-Jenkins
Copy link

Can one of the admins verify this patch?

@chrisdickinson
Copy link

Closing this -- the patch is way out of date with the v0.12 branch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants