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

frint-di: move it into monorepo #403

Merged
merged 6 commits into from
Jan 15, 2018
Merged

frint-di: move it into monorepo #403

merged 6 commits into from
Jan 15, 2018

Conversation

fahad19
Copy link
Member

@fahad19 fahad19 commented Jan 14, 2018

What's done

  • Imported travix-di (which is a fork of diyai), into the monorepo

Why

  • The package was previously left almost unattended
  • Unit tests coverage was not up to the standards of the monorepo
  • It is the base of frint package itself as a dependency and extremely important
  • This should also help us migrate to TypeScript swiftly

Before merge

  • Check coverage

@fahad19 fahad19 self-assigned this Jan 14, 2018
@fahad19 fahad19 requested a review from a team January 14, 2018 15:54
@codecov
Copy link

codecov bot commented Jan 14, 2018

Codecov Report

Merging #403 into master will increase coverage by 0.05%.
The diff coverage is 99.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #403      +/-   ##
==========================================
+ Coverage   97.39%   97.45%   +0.05%     
==========================================
  Files         101      105       +4     
  Lines        3992     4120     +128     
==========================================
+ Hits         3888     4015     +127     
- Misses        104      105       +1
Impacted Files Coverage Δ
packages/frint-di/src/resolveContainer.spec.js 100% <100%> (ø)
packages/frint-di/src/resolveContainer.js 100% <100%> (ø)
packages/frint/src/App.js 92.1% <100%> (ø) ⬆️
packages/frint-di/src/createContainer.spec.js 100% <100%> (ø)
packages/frint-di/src/createContainer.js 97.36% <97.36%> (ø)

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 9275698...58d3464. Read the comment docs.

@fahad19 fahad19 added minor and removed patch labels Jan 14, 2018
@markvincze
Copy link
Contributor

The index.d.ts file was not correct in travix-di, you can replace it with this: https://gist.github.com/markvincze/d4cbf940687f676bcf6b4e19c7a7b0f3 (and remove createClass if that's not there any more).

@fahad19
Copy link
Member Author

fahad19 commented Jan 14, 2018

Thanks for the gist, @markvincze!

I am probably doing something wrong myself, but updating the typings is now failing the build for some reason. I will investigate further from office tomorrow.

@fahad19
Copy link
Member Author

fahad19 commented Jan 14, 2018

It was failing in $ npm run bootstrap.

I removed the I prefix from typings, and it started working again.

Cannot confirm about idiomatic TypeScript myself, but there is this Issue here that may help understand more: microsoft/TypeScript-Handbook#121

@fahad19 fahad19 merged commit 37fb2e2 into master Jan 15, 2018
@fahad19 fahad19 deleted the frint-di branch January 15, 2018 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants