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

Fix building with node 10.0.0 #71

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

Paxa
Copy link

@Paxa Paxa commented Jul 7, 2018

I found this useful patch in #68 (i'm not an author of this changes)

Thank you @stormwin

@stormwin
Copy link

stormwin commented Jul 8, 2018

If i have more free time, i will fix backward incompatibilities with older nodejs versions. The problem is with OpenSSL version that is changed from 1.0.x to 1.1.x in nodeJS v10.x

@stormwin
Copy link

stormwin commented Jul 8, 2018

OK, finally it work on every nodejs version.

@Paxa
Copy link
Author

Paxa commented Jul 9, 2018

Bravo!

@jacobpdq
Copy link

@Southern bump plz (needed for node-solid)

@alexanderkhivrych
Copy link

plz merge it

@alexanderkhivrych
Copy link

ASAP

@alexanderkhivrych
Copy link

alexanderkhivrych commented Jul 19, 2018

build already green

@alexanderkhivrych
Copy link

@stormwin

@stormwin
Copy link

I am also waiting for this merge. Till then, i am using my own fork in my projects

@jacobpdq
Copy link

jacobpdq commented Jul 19, 2018 via email

@alexanderkhivrych
Copy link

@Southern

@Paxa
Copy link
Author

Paxa commented Jul 23, 2018

Guys, you can just use it from github: yarn add stormwin/node-x509 or npm i stormwin/node-x509

@yorkie
Copy link
Collaborator

yorkie commented Jul 28, 2018

@Paxa Thanks for this patch very much, may I ask you not to change the coding style, such that we could merge them only with building patch :)

@yorkie yorkie mentioned this pull request Jul 28, 2018
@Paxa
Copy link
Author

Paxa commented Jul 28, 2018

@yorkie I just created a PR from @stormwin fork

@eduardocruz
Copy link

Did the project die?

@adamhamlin
Copy link

Bump

@mbwhite
Copy link
Contributor

mbwhite commented Nov 16, 2018

Can this be merged please?

Copy link
Collaborator

@yorkie yorkie left a comment

Choose a reason for hiding this comment

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

@Paxa Thanks, but I found you changed the indent to a tab or 4 spaces which could make the source is hard to review, please remove those coding style changes.

Local<Value> try_parse(const std::string& dataString);
Local<Value> verify(const std::string& dataString);
Local<Value> try_parse(const std::string &dataString);
Local<Value> verify(const std::string &dataString);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please revert these style changes?

Copy link

Choose a reason for hiding this comment

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

i wish someone just edited these files using github inline edit :)

char* real_name(char *data);
char* trim(char *data, int len);
char *real_name(char *data);
char *trim(char *data, int len);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto.

Nan::Set(exports, Nan::New<String>("getIssuer").ToLocalChecked(),
Nan::New<FunctionTemplate>(get_issuer)->GetFunction());
Nan::Set(exports, Nan::New<String>("parseCert").ToLocalChecked(),
Nan::New<FunctionTemplate>(parse_cert)->GetFunction());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use the 2 spaces as indents.

@yorkie
Copy link
Collaborator

yorkie commented Nov 17, 2018

Can this be merged please?

@mbwhite Sorry for the late, already leave comments here though the CI is pass :)

@stormwin
Copy link

OK, i will try to fix this styling stuffs and also will try to fix some warnings for node v11. Will do a new commits today or tomorrow

@ghost
Copy link

ghost commented Nov 23, 2018

@stormwin you awesome!
thanks for fixing this,
hoping to see it merged

@okonon
Copy link

okonon commented Nov 30, 2018

Bump. @stormwin any chance that you fixed that styling ?

yorkie pushed a commit that referenced this pull request Dec 19, 2018
This is the #71 but with corrected formatting as per the discussion in #71 
Thank you to those who worked on fix; aim here is to get the fix merged as it is needed by
many projects

Signed-off-by: Matthew B. White <[email protected]>
@okonon
Copy link

okonon commented Jan 11, 2019

Anyone can resolve the conflicts?

@prompt-bot
Copy link

there any follow up?

@mbwhite
Copy link
Contributor

mbwhite commented Mar 17, 2019

there any follow up?

We would have preferred to use this repo - but due to schedules we had to fork and republished to npm.

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.

10 participants