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

feat(*): Add Typescript type definitions #786

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

j4m3sb0mb
Copy link

Signed-off-by: Giacomo Minighin [email protected]

Issue #785

Added typescript definitions for ergo-compiler ergo-engine

Changes

  • added packages/ergo-compiler/types/index.d.ts
  • added packages/ergo-compiler/types/tsconfig.json
  • added typescript @types/node in ergo-compiler dev dependencies
  • added script types:check in ergo-compiler
  • added packages/ergo-engine/types/index.d.ts
  • added packages/ergo-engine/types/tsconfig.json
  • added typescript in ergo-engine dev dependencies
  • added script types:check in ergo-engine

Signed-off-by: Giacomo Minighin <[email protected]>
@mttrbrts
Copy link
Sponsor Member

mttrbrts commented Mar 2, 2021

@j4m3sb0mb My apologies that it has taken us so long to get to this. Thank you for your contribution.

I would love to get this merged, can you please remove the ^ symbols from the changed package.json files. In this repository we require that all dependencies use explicit versions.

You can check that the test passes locally by running npm run depcheck from the root folder.

Signed-off-by: Giacomo Minighin <[email protected]>
@coveralls
Copy link

coveralls commented Mar 2, 2021

Coverage Status

Coverage remained the same at 95.376% when pulling a6b5229 on j4m3sb0mb:master into 653df8d on accordproject:master.

Signed-off-by: Giacomo Minighin <[email protected]>
@mttrbrts
Copy link
Sponsor Member

mttrbrts commented Mar 5, 2021

@jeromesimeon @radhikakotangoor I'm happy with this, do either of you have input on the type definitions?

@mttrbrts mttrbrts changed the title types definitions feat(*): Add Typescript type definitions Mar 5, 2021
@j4m3sb0mb
Copy link
Author

I left some any type that should be changed with the proper type but I wasn't sure wich was.

Copy link
Member

@jeromesimeon jeromesimeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic contribution. Thanks for much @j4m3sb0mb !

Just a couple of things:

  1. we'll need a rebase to merge this I'm afraid, due to conflict with the latest version in master (I could take this over, but would prefer if you manage to do it since it's your code)
  2. I would really love a mini "helloworld.ts" showing how anyone can use this from typescript. This might serve as a minimal test as well

@j4m3sb0mb
Copy link
Author

Ok @jeromesimeon where should i put the "helloworld.ts"?

@jeromesimeon
Copy link
Member

Ok @jeromesimeon where should i put the "helloworld.ts"?

That's a really good question. I would put it in e.g., ./tests/typescript for now. We can see later if we find a better place.

@j4m3sb0mb
Copy link
Author

I'm writing a little example with some functions but I think that for a real test should be used this package: https://github.com/SamVerschueren/tsd

@jeromesimeon
Copy link
Member

I'm writing a little example with some functions but I think that for a real test should be used this package: https://github.com/SamVerschueren/tsd

Hi @j4m3sb0mb it's been a while since I've looked at TypeScript, but what you are suggesting sounds good (we will have to maintain this over time). I think the PR is valuable as is though I think I would be happy with either:

  1. add a small example to explain how to use the code with TypeScript and merge this, then do a later separate PR with a testing package along the lines of the one you suggest
  2. keep this PR open and wait for testing

I think it's a bit up to you which of those sound better.

Thanks!

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

Successfully merging this pull request may close these issues.

4 participants