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(core-interfaces): initial implementation #1924

Merged
merged 19 commits into from
Jan 3, 2019
Merged

feat(core-interfaces): initial implementation #1924

merged 19 commits into from
Jan 3, 2019

Conversation

paroxysm
Copy link
Contributor

@paroxysm paroxysm commented Jan 2, 2019

Proposed changes

Resolves #1783

The gist of this PR is captured in the discussion I had with @faustbrian in the issue above.
The core modules innately have deps on each other, so packaging and exporting the types for each module cannot work because we'll introduce circular deps. The same issue arises if you create separate types packages for each module, those types packages will end up referencing each other, creating yet another circular dependency.
So I've created an interfaces package, you can define interfaces that can ref other interfaces and vice-versa, but since they live in one package, you don't run into a circular dependency issue.

So far, I've created/implemented interfaces for core-container, core-blockchain, core-logger, core-event-emitter, core-p2p, core-transaction-pool.
I've avoid core-database because there's some consolidation that needs to happen between core-database and core-database-postgres.

The stipulation with this new package is that, only interfaces of core modules and only interfaces/classes that are referenced across modules are added. If an interface/class doesn't have to be visible outside its module to be used, then it doesn't get added. This prevents bloating up of the core-interfaces module

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build (changes that affect the build system)
  • Docs (documentation only changes)
  • Test (adding missing tests or fixing existing tests)
  • Other... Please describe:

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@faustbrian faustbrian requested review from faustbrian and spkjp January 2, 2019 04:07
Copy link
Contributor

@faustbrian faustbrian left a comment

Choose a reason for hiding this comment

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

All *Impl should keep their original names and extend interfaces that are prefixed with I instead.

@paroxysm
Copy link
Contributor Author

paroxysm commented Jan 3, 2019

@faustbrian I would like to move the

get p2p() { return app.resolvePlugin<P2P.IMonitor>("p2p"); }
get transactionPool() { return app.resolvePlugin<TransactionPool.ITransactionPool>("transactionPool"); }

And after consolidation
get database() { return app.resolvePlugin<PostgresConnection>("database"); }

out of blockchain.ts and into IContainer & Container respectively, also add a blockchain(), logger() getters as well.
Additionally, change all references of ".resolvePlugin<>" calls to use the respective method
So instead of app.resolvePlugin<ILogger> you use app.logger() or app.blockchain() or app.database()
It's less verbose, removes the need to go through Blockchain to get at those( i.e. see blockchain.test.ts, ) and also helps hide some implementation details(i.e. we solve instances in awilix by a string key. Maybe in the future if change how we lookup stuff or we want a different container implementation, it's a lot of places to go back and refactor.

@faustbrian
Copy link
Contributor

A future refactor of the core-container package was going to include such changes but in a different manor then simply throwing it into the container.

Right now the container is basically the application object and knows about things it shouldn't be concerned with like what config/network we are going to use, our application version and if the application is shutting down or not.

Those kinda information should be stored in a class Application {} that exposes some of the container functions through its public API and that class would also provide getter/setter functions for core components like logging, database access and blockchain.

@paroxysm
Copy link
Contributor Author

paroxysm commented Jan 3, 2019

Agreed, that's a better design

@ghost ghost assigned faustbrian Jan 3, 2019
@ghost ghost added the review label Jan 3, 2019
@faustbrian
Copy link
Contributor

@paroxysm can you retrigger a build on CircleCI, it failed because of some node-gyp build error.

@faustbrian faustbrian changed the title WIP: Add & Use core-interfaces module feat(core-interfaces): initial implementation Jan 3, 2019
@faustbrian faustbrian merged commit dfa3881 into ArkEcosystem:develop Jan 3, 2019
@ghost ghost removed the review label Jan 3, 2019
@paroxysm
Copy link
Contributor Author

paroxysm commented Jan 3, 2019

That node-gyp brittleness is annoying. I wonder if there's a permanent solution for it, I've never seen it locally(probably because I'm not starting with a clean-slate everytime), it seems to be a compilation issue with that library....

@paroxysm paroxysm deleted the feat/add-core-interfaces-module branch January 3, 2019 17:35
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.

2 participants