MadKudu / node-hubspot

Node wrapper for the HubSpot API
MIT License
192 stars 157 forks source link

Move to Typescript #156

Open AustinLeeGordon opened 5 years ago

AustinLeeGordon commented 5 years ago

@AustinLeeGordon looks like we were able to move past the prettier issue but now it is telling me there is another error...which i believe is another prettier issue

Detailed stack trace: /user_code/node_modules/hubspot/lib/oauth.js:12 ...data, ^^^

SyntaxError: Unexpected token ... at createScript (vm.js:56:10)

Originally posted by @PrimalIan in https://github.com/MadKudu/node-hubspot/issues/154#issuecomment-461984645

AustinLeeGordon commented 5 years ago

@pcothenet thoughts on adding Babel to fix issues like this?

pcothenet commented 5 years ago

@AustinLeeGordon @PrimalIan, so according to https://node.green/#ESNEXT-candidate--stage-3--object-rest-spread-properties, the spread operator is fully supported as of node 8? (but not node 6). Is that correct?

I'm not sure what the usage stats out there are? Keeping support for node 6 seems reasonable, so no objection with adding a babel transpiling. I'm pretty short on time though (as you may have seen). @AustinLeeGordon want to give it a shot?

pcothenet commented 5 years ago

If it's just ..., a simpler alternative might just to revert the part of this commit: https://github.com/MadKudu/node-hubspot/commit/fbb24556c6b4b0d9cd448a3e0ec5b1bb19525ce1#diff-84fad1d3b3913ec86805e857580c5d54

And go back to using _.extend (or rather the native Object.assign) instead of...`

What do you think?

@Primallan, what version of node.js are you using?

AustinLeeGordon commented 5 years ago

@pcothenet yeah, I was thinking 6 didn’t support it. I don’t mind giving it a shot. I’d rather see it being tranpiled than constantly having to go back and fix any future commits with something that node v6 doesn’t support.

AustinLeeGordon commented 5 years ago

@pcothenet Haven't gotten into TypeScript yet, but after looking into it and Babel a little bit, does it make sense to switch over to TS and leave out Babel? From what I've read about it, TS can transpile to es5.

pcothenet commented 5 years ago

Typescript transpiles well to any target version of es5 (I've done many internal lib in typescript). This would also help to avoid having to maintain internal types.

In tsconfig.json, you want something like this:

{
  "compilerOptions": {
    "declaration": true,
    "outDir": "./dist/",
    "sourceMap": true,
    "module": "commonjs",
    "lib": [
      "es6"
    ],
    "target": "es6"
  },
  "include": [
    "./lib/**/*"
  ],
  "exclude": [
    "node_modules",
    "dist"
  ]

and in package.json, you have something like this:

{
  "main": "./dist/index.js",
  "types": "./dist/index.d.ts",
  "files": [
    "dist/**/*"
  ]
}

(which tells npm to publish the dist and links users of your library to the correct file).

TBH, I don't have time to do the full typescript thing right now but if you're up for it, I'll be happy to spare some time to test and unblock you if needed

AustinLeeGordon commented 5 years ago

@pcothenet Thanks! I've been meaning to learn it, so I'd love to give it a shot.

AustinLeeGordon commented 5 years ago

Want to re-open #114 and close this one or just keep it going in here?

pcothenet commented 5 years ago

Let's keep it going here and rename.

AustinLeeGordon commented 5 years ago

@pcothenet For contacts, the property and property groups are both in the contact.property method. Should the ones for groups be put into a contact.property.group method similar to how company and deal are structured?

pcothenet commented 5 years ago

That's a very good point. Since changing the method would be a breaking change, do you think it'd be worth moving to a similar structure but keeping the previous aliases (to avoid a breaking change).

Otherwise, we can introduce a breaking change but that's always slightly annoying for users.

We might also want to consider separating the two issues (unless you think it adds unnecessary extra work)

AustinLeeGordon commented 5 years ago

Yeah, I figured I should ask since it was breaking. There were a few other things that needed to be addressed similar to that. Some depreciated endpoints; methods that accepted options but didn’t actually have any; some methods named get, getAll, getByName, getById,getOne`, etc. that could be normalized. I’ve got some more notes and details on my computer about possible changes. Writing this on my phone right now, so that’s just what I can remember off the top of my head.

Would be good to go ahead and update it, but it might be better to break it down. I’ll post a progress update and let you know where I have questions here soon!