Skip to content
This repository has been archived by the owner on Oct 19, 2023. It is now read-only.

Move to Typescript #156

Open
AustinLeeGordon opened this issue Feb 9, 2019 · 12 comments
Open

Move to Typescript #156

AustinLeeGordon opened this issue Feb 9, 2019 · 12 comments

Comments

@AustinLeeGordon
Copy link
Contributor

@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)

@AustinLeeGordon
Copy link
Contributor Author

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

@pcothenet
Copy link
Contributor

@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
Copy link
Contributor

pcothenet commented Feb 9, 2019

If it's just ..., a simpler alternative might just to revert the part of this commit: fbb2455#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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Contributor

pcothenet commented Feb 9, 2019

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
Copy link
Contributor Author

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

@AustinLeeGordon
Copy link
Contributor Author

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

@pcothenet
Copy link
Contributor

Let's keep it going here and rename.

@pcothenet pcothenet changed the title Transpile with Babel Move to Typescript Feb 13, 2019
@AustinLeeGordon
Copy link
Contributor Author

AustinLeeGordon commented Feb 13, 2019

@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
Copy link
Contributor

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
Copy link
Contributor Author

AustinLeeGordon commented Feb 15, 2019

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!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants