Skip to content

Export & use core-container types#1891

Merged
faustbrian merged 19 commits intoArkEcosystem:developfrom
paroxysm:feat/export-core-container-types
Dec 21, 2018
Merged

Export & use core-container types#1891
faustbrian merged 19 commits intoArkEcosystem:developfrom
paroxysm:feat/export-core-container-types

Conversation

@paroxysm
Copy link
Copy Markdown
Contributor

Proposed changes

Address #1789 and also further propagate core-logger's AbstractLogger type usage.
We're leveraging ts generics to be able to retrieve typed objects directly from the Container class.
For the time-being, the generic is defaulted to any to prevent compilation issues, but in the future, as we expose more module typedefs, we can remove that default to enforce that we always get typed values from the Container

We're also introducing core-logger as a dependency to every module that was previously grabbing a reference via container.resolvePlugin('logger')

I'm trying to be careful not to introduce a circular dependency. I know there can definitely be one in core-container and core-logger when they both try to import each other's types. For this edge-case we can break the pattern by having core-logger not import core-container's typedefs but it's not that big of an issue.

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)

@ghost ghost assigned faustbrian Dec 21, 2018
@ghost ghost added the review label Dec 21, 2018
@faustbrian
Copy link
Copy Markdown
Contributor

@paroxysm some failing tests after merging develop

@paroxysm
Copy link
Copy Markdown
Contributor Author

Looking into them..

@paroxysm
Copy link
Copy Markdown
Contributor Author

paroxysm commented Dec 21, 2018

@faustbrian These failures don't appear to have anything to do w/ my changes. There is brittleness in there somewhere that's surfacing now. The code that was merged doesn't even affect the module whose tests are failing.
Some CI's are failing while some are passing demonstrating just how finicky those tests are..

@paroxysm
Copy link
Copy Markdown
Contributor Author

paroxysm commented Dec 21, 2018

@faustbrian I can reproduce these failures on the develop branch w/o the changes in this PR. Seems something else recent is causing these failures.

@paroxysm
Copy link
Copy Markdown
Contributor Author

Nevermind, CI was reporting some very weird errors that led me astray. I see what's caused this and fixed it

@paroxysm
Copy link
Copy Markdown
Contributor Author

It's strange tho, there should've been a 100% failure across all CI runs but only a few failed while some passed....

@faustbrian faustbrian merged commit 26374df into ArkEcosystem:develop Dec 21, 2018
@ghost ghost removed the review label Dec 21, 2018
@faustbrian
Copy link
Copy Markdown
Contributor

faustbrian commented Dec 21, 2018

@paroxysm you can use https://help.github.com/articles/closing-issues-using-keywords/ if you want to automatically close issues as resolved. Instead of Address #1789 write Resolves #1789.

Not all CircleCI jobs run the same tests and not all have the resolution issues so it is normal that only certain ones fail.

@paroxysm
Copy link
Copy Markdown
Contributor Author

paroxysm commented Dec 21, 2018

Thanks, that's really nifty @faustbrian!
And mm, that makes sense, I haven't peered closely at those configs to notice a difference... That makes sense.

@paroxysm paroxysm deleted the feat/export-core-container-types branch December 21, 2018 23:36
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