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

v2 Release #174

Closed
5 of 6 tasks
JCMais opened this issue May 19, 2019 · 5 comments
Closed
5 of 6 tasks

v2 Release #174

JCMais opened this issue May 19, 2019 · 5 comments

Comments

@JCMais
Copy link
Owner

JCMais commented May 19, 2019

What is missing for v2 release:

@JCMais
Copy link
Owner Author

JCMais commented May 27, 2019

I will leave the API docs for later, the examples should be self-documenting. If anyone has suggestions for the examples, let me know.

I will release v2 this weekend.

@gschier
Copy link

gschier commented May 29, 2019

I just started working on an upgrade to v2 today. Everything seems to have gone smoothly and it only took a few minutes 😄

The main thing I noticed with the migration is that some of the naming seems inconsistent now. For example, the change from SSL_DATA_IN became SslDataIn but most constants still remain in UPPER_CASE format? Will you be converting all constants to this new naming scheme?

Also related, I noticed you have Curl.option and CurlAuth which are also inconsistent in style.


Feel free to ignore this comment if it's still a work in progress. Looking forward to v2!

@JCMais
Copy link
Owner Author

JCMais commented May 29, 2019

hey @gschier, great to hear the upgrade was easy!

Curl.option, Multi.option, Share.option and Curl.info will keep their current casing, mostly to keep them almost exactly like the options on libcurl itself (basically just dropping the prefix, CURLOPT, CURLMOPT, etc).

The newly introduced symbols will be in CamelCase, like CurlAuth, the reason is that those are now Typescript enums, and that is the recommended convention for them: https:/JCMais/node-libcurl/tree/develop/lib/enum (and I think it does looks nice)

However, there are a few exceptions I made to that convention, they are:

  • CurlCode, CurlMultiCode, and CurlShareCode
    Those were previously on Curl.code (Curl.code.CURLE_OK become CurlCode.CURLE_OK), they are still using the UPPER_CASE casing to make it easier to look for them, since they are exactly like the constants defined on libcurl itself.
  • CurlProtocol (previously Curl.protocol) -> https:/JCMais/node-libcurl/blob/32647acc28b026bbd03aa1e3abea9cbbc8f7d43b/lib/enum/CurlProtocol.ts
    I felt it was weird to have the protocols on camel case.

Another point it's important to mention about the options, there is one place you are allowed to pass them in camel case, the new async/await wrapper curly, there is a programmatically generated camelCase -> UPPER_CASE mapping (that is, it probably has some weird options names I didn't caught initially) here:

export const CurlOptionCamelCaseMap = {

The motivation behind that is that I really disliked passing UPPER_CASE string as object properties 😄

const { curly } = require('node-libcurl')

// ... async function context:
const { data, headers, statusCode } = await curly.get('https://www.google.com', {
  followLocation: true,
})

// same than:
const { data, headers, statusCode } = await curly.get('https://www.google.com', {
  FOLLOW_LOCATION: true,
})

@JCMais
Copy link
Owner Author

JCMais commented Jun 1, 2019

any others suggestions / issues found during the upgrade @gschier (or anyone that have upgraded)?

@JCMais
Copy link
Owner Author

JCMais commented Jun 3, 2019

v2 has been released! 🎉

@JCMais JCMais closed this as completed Jun 3, 2019
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

No branches or pull requests

2 participants