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

refactor: completely replace d2 by data engine #230

Merged
merged 26 commits into from
Nov 5, 2020

Conversation

HendrikThePendric
Copy link
Contributor

@HendrikThePendric HendrikThePendric commented Oct 26, 2020

  • 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

@HendrikThePendric HendrikThePendric force-pushed the DHIS2-9700-remove-d2-and-switch-to-engine branch from e4cef98 to 5c24aa0 Compare October 28, 2020 16:05
@varl
Copy link
Contributor

varl commented Oct 30, 2020

Fix in app-shell for build failure: dhis2/app-platform#475

@varl varl force-pushed the DHIS2-9700-remove-d2-and-switch-to-engine branch from 7858f67 to 5aaf080 Compare October 30, 2020 14:10
src/actions/index.js Outdated Show resolved Hide resolved
src/api/api.js Show resolved Hide resolved
src/api/api.js Outdated Show resolved Hide resolved
src/api/api.js Show resolved Hide resolved
src/api/api.js Outdated Show resolved Hide resolved
src/api/api.js Show resolved Hide resolved
src/api/api.js Outdated Show resolved Hide resolved
src/api/api.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ismay ismay left a 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?

@HendrikThePendric
Copy link
Contributor Author

  • Why was index.js added in several imports? I'm used to omitting that since it's the default when importing a folder.

See this Slack comment and also the conversation above and below it. We prefer explicit over implicit.

  • With a large change like this it seems good to me to increase test coverage, especially in the affected areas. What do you think?

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 DataQuery/DataMutation. Probably I should have added tests prior to refactoring this. The main benefit of these tests IMO would be to catch regressions introduced by the refactor. I did not do so, and I wonder if adding these tests now would add much benefit since I've manually tested for regressions already. Given the fact that (the vast majority of) the changes are in the api-module, I think just checking if the date-engine-based version of a function produces the same request as the d2 based is probably sufficient. So what I am suggesting is to not add tests now, but add tests when we do the bigger refactor and switch from REDUX to the DataQuery/DataMutation approach.

@ismay ismay self-requested a review November 5, 2020 13:59
@HendrikThePendric HendrikThePendric merged commit e8b2279 into master Nov 5, 2020
@HendrikThePendric HendrikThePendric deleted the DHIS2-9700-remove-d2-and-switch-to-engine branch November 5, 2020 14:10
@HendrikThePendric HendrikThePendric restored the DHIS2-9700-remove-d2-and-switch-to-engine branch November 5, 2020 14:10
@HendrikThePendric HendrikThePendric deleted the DHIS2-9700-remove-d2-and-switch-to-engine branch November 5, 2020 14:11
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants