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

feat(*): Add Typescript type definitions#786

Open
j4m3sb0mb wants to merge 3 commits intoaccordproject:mainfrom
j4m3sb0mb:master
Open

feat(*): Add Typescript type definitions#786
j4m3sb0mb wants to merge 3 commits intoaccordproject:mainfrom
j4m3sb0mb:master

Conversation

@j4m3sb0mb
Copy link
Copy Markdown

Signed-off-by: Giacomo Minighin giacomo.minighin@gmail.com

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 <giacomo.minighin@gmail.com>
@mttrbrts
Copy link
Copy Markdown
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 <giacomo.minighin@gmail.com>
@coveralls
Copy link
Copy Markdown

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 <giacomo.minighin@gmail.com>
@mttrbrts
Copy link
Copy Markdown
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
Copy Markdown
Author

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

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

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

@jeromesimeon
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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