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

feat: typescript and tests #9

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@adrianhopebailie
Copy link
Contributor

commented Sep 11, 2018

  • update to TypeScript
  • add tests and coverage
  • deprecate inconsistent behaviour

adrianhopebailie added some commits Sep 11, 2018

@codecov-io

This comment has been minimized.

Copy link

commented Sep 11, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@a8338fe). Click here to learn what that means.
The diff coverage is 75.67%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master       #9   +/-   ##
=========================================
  Coverage          ?   75.67%           
=========================================
  Files             ?        1           
  Lines             ?       37           
  Branches          ?       12           
=========================================
  Hits              ?       28           
  Misses            ?        4           
  Partials          ?        5
Impacted Files Coverage Δ
src/index.ts 75.67% <75.67%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8338fe...323173e. Read the comment docs.

@adrianhopebailie adrianhopebailie requested a review from sentientwaffle Sep 11, 2018

@emschwartz
Copy link
Member

left a comment

Looks great! Just two minor comments

README.md Outdated
run()
```

If no parameters are provided it will attempt to find the config in environment variables. If these are not found it will load a plugin that attempts to connect to a local moneyd instance on port 7768.

This comment has been minimized.

Copy link
@emschwartz

emschwartz Sep 12, 2018

Member

Link to moneyd

}
export type DataHandler = (data: Buffer) => Promise<Buffer>
export type MoneyHandler = (amount: string) => Promise<void>
export interface PluginInstance extends EventEmitter {

This comment has been minimized.

Copy link
@emschwartz

emschwartz Sep 12, 2018

Member

I think this type should just be called Plugin

This comment has been minimized.

Copy link
@adrianhopebailie

adrianhopebailie Sep 12, 2018

Author Contributor

I'm trying to get some consistency across all our "module loading" types so I copied the StoreInstance type name from ilp-connector.

I also want to avoid using very common names.

Maybe a better convention is IlpPlugin and IlpStore etc?

This comment has been minimized.

Copy link
@emschwartz

emschwartz Sep 12, 2018

Member

I don't think having a common name is a problem here because a) you don't need to import this interface b) if you do you're going to use it for the main plugin abstraction and c) if it does conflict with something else you've called Plugin you can always rename it. IlpPlugin seems a little redundant because you're getting it from a module called ilp something so there isn't any ambiguity about the context

@adrianhopebailie adrianhopebailie force-pushed the feat/typescript-and-tests branch from ba6afc9 to 323173e Sep 12, 2018

run()
```

If no parameters are provided it will attempt to find the config in environment variables. If these are not found it will load a plugin that attempts to connect to a local [moneyd](../moneyd) instance on port 7768.

This comment has been minimized.

Copy link
@emschwartz

emschwartz Sep 12, 2018

Member

I meant link to the moneyd repo so you can find instructions for running it (right now the link is broken)

use this anywhere that you need an ILP plugin created from details in the
environment.
environment or by specifying the detauls in code.

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Sep 12, 2018

Contributor

s/detauls/defaults

constructor of `ilp-plugin-btp` or the module name in `ILP_PLUGIN`.
TypeScript:
```typescript
import createPlugin, { DataHandler } from '..'

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Sep 12, 2018

Contributor

Since this is a README example, should it import from ilp-plugin instead of a relative path?

run()
```

If no parameters are provided it will attempt to find the config in environment variables. If these are not found it will load a plugin that attempts to connect to a local [moneyd](../moneyd) instance on port 7768.

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Sep 12, 2018

Contributor

Maybe say "connect to a local moneyd instance on port 7768 using ilp-plugin-btp", just to be explicit about the default plugin.


async function run () {
await plugin.connect()
await plugin.sendData(/* ... */)

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Sep 12, 2018

Contributor

Is this example intended to be runnable? If so, there should be some dummy data here.

"version": "3.2.1",
"description": "> A generic handle to ILP",
"main": "index.js",
"version": "3.2.0",

This comment has been minimized.

Copy link
@sentientwaffle

sentientwaffle Sep 12, 2018

Contributor

What's with the version change here?


export interface PluginServices {
store: any
log: Logger

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Apr 23, 2019

Logger needs to be imported from ilp-logger.
Could use the latest version of ilp-logger (which has Typescript) while you're at it.

@sharafian

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2019

Is this still active at all? Can it be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.