-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor: completely replace d2 by data engine #230
refactor: completely replace d2 by data engine #230
Conversation
HendrikThePendric
commented
Oct 26, 2020
•
edited
Loading
edited
- This is a redux app so I opted for using the data engine imperatively in the api module that already existed
- The majority of the changes are in that module, but GitHub will collapse this file, so be aware of that
- Some component changes were introduced as a result of the removal of the d2 adapter
- Other component changes were triggered by the fact that some things, like the current-user system version were previously read from the d2 instance
8cffe18
to
9e3a1b3
Compare
e4cef98
to
5c24aa0
Compare
Fix in app-shell for build failure: dhis2/app-platform#475 |
7858f67
to
5aaf080
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a couple comments. In general I was also wondering:
- Why was
index.js
added in several imports? I'm used to omitting that since it's the default when importing a folder. - With a large change like this it seems good to me to increase test coverage, especially in the affected areas. What do you think?
See this Slack comment and also the conversation above and below it. We prefer explicit over implicit.
Yeah in general I agree. I definitely think I will be adding tests when I try to remove redux from the app and move to the |
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |