-
-
Notifications
You must be signed in to change notification settings - Fork 27
feat(tls.createSecureContext): introduce #156
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
Conversation
AugustinMauroy
left a comment
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.
Minor thing
Co-authored-by: Bruno Rodrigues <[email protected]>
Co-authored-by: Bruno Rodrigues <[email protected]>
Co-authored-by: Bruno Rodrigues <[email protected]>
brunocroh
left a comment
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.
You did a great job on that, thank you for your awesome contribution!
I have some comments on a few points, feel free to share your opinion if you don't agree with something.
And just one more thing: could you please check if you ran node --run pre-commit? I think there are some Biome rules that aren't applied.
Co-authored-by: Bruno Rodrigues <[email protected]>
Co-authored-by: Augustin Mauroy <[email protected]>
Co-authored-by: Bruno Rodrigues <[email protected]>
AugustinMauroy
left a comment
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.
LGMT ! Thanks for you first contribution.
If you have any feedback about DX on this project let's me know. And if you have any feedback about codemod CLI/platform I'm going to transfer it to them
Co-authored-by: Augustin Mauroy <[email protected]>
Co-authored-by: Augustin Mauroy <[email protected]>
Co-authored-by: Augustin Mauroy <[email protected]>
|
fro windows failing use https://nodejs.org/api/os.html#oseol instead of |
|
we also have few utility you should use it to simplify |
Thanks! I'm sure it was a fair amount of work 😅 (that's why I said it wasn't necessarily a blocker) Unfortunately you force-pushed, which wipes the review history. That means the entire PR has to get reviewed again instead of just the pieces recently changed (and that massively increases the effort & time needed to review the latest changes). I've updated the repo settings to now block force-pushing to prevent this happening again (you weren't the only one to do it), but FYI the next review will take me some time to get through now because of that force-push 😬 |
AugustinMauroy
left a comment
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.
LGMT !
JakobJingleheimer
left a comment
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.
LGTM :)
PS Sorry it took me a while to get back to this.
recipes/createCredentials-to-createSecureContext/src/workflow.ts
Outdated
Show resolved
Hide resolved
recipes/createCredentials-to-createSecureContext/src/workflow.ts
Outdated
Show resolved
Hide resolved
recipes/createCredentials-to-createSecureContext/src/workflow.ts
Outdated
Show resolved
Hide resolved
recipes/createCredentials-to-createSecureContext/src/workflow.ts
Outdated
Show resolved
Hide resolved
recipes/createCredentials-to-createSecureContext/src/workflow.ts
Outdated
Show resolved
Hide resolved
|
hey @0hmX any news on this pr ? |
|
@0hmX did you intend to close this? O.o it's ready to go |
|
I'm going to take over this pr so we can land it |
|
@AugustinMauroy CI here is failing |
Update package-lock.json Update package-lock.json Update package-lock.json
8d05509 to
bfb02a3
Compare
|
LGTM |
closes: #123