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

migrate code base to type script #107

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

zeevdr
Copy link

@zeevdr zeevdr commented Mar 8, 2023

I've added the ability yo write nodeodm code in typescript. still need to update documentation

@zeevdr zeevdr marked this pull request as ready for review April 4, 2023 14:31
@tariqislam
Copy link

@pierotofy Any comments on this PR?

@smathermather
Copy link
Contributor

smathermather commented May 15, 2023

@zeevdr -- I haven't tested yet, but can you fix the branch merge conflicts for libs/asr-providers/aws.js and libs/proxy.js?

Also, it looks like there's a lot of boiler plate still in tsconfig.json. If this is normal, by all means leave it. I don't know conventions here, and often with configs, we leave the possible options in the comments. If it typical to clean this up, feel free to reduce that to only what is needed. But as I say: I don't know typescript conventions, so whichever fits with the larger ecosystem conventions.

@smathermather smathermather requested a review from pierotofy May 19, 2023 17:01
@pierotofy
Copy link
Member

pierotofy commented May 19, 2023

What the goal is:

  • A migration to TypeScript (for the obvious benefits of static typing and tooling)

What we currently have in this PR:

  • A skeleton for the beginning of the goal above (no actual code has been migrated to take advantage of TypeScript's features)

Simply enabling TypeScript support will just remove compatibility with vanilla JS (a net negative). Happy to leave this in a typescript branch, but as-is this cannot be merged into master (even after resolving conflicts).

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

Successfully merging this pull request may close these issues.

5 participants