-
Notifications
You must be signed in to change notification settings - Fork 157
Move to Typescript #156
Comments
@pcothenet thoughts on adding Babel to fix issues like this? |
@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? |
If it's just And go back to using What do you think? @Primallan, what version of node.js are you using? |
@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. |
@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. |
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 {
"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 |
@pcothenet Thanks! I've been meaning to learn it, so I'd love to give it a shot. |
Want to re-open #114 and close this one or just keep it going in here? |
Let's keep it going here and rename. |
@pcothenet For contacts, the property and property groups are both in the |
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) |
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 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! |
@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 #154 (comment)
The text was updated successfully, but these errors were encountered: