play-co / devkit-core

11 stars 17 forks source link

Added analytics + removal of logic related to native build #69

Closed bchevalier closed 7 years ago

bchevalier commented 7 years ago

Changes overview

This PR consists in 2 main changes: 1 - The addition of the analytics component (previously located on game repos) 2 - The removal of the logic responsible for generating native builds

Reason for changes

1 - To ensure the uniqueness of the analytics component across all games (to unify the behavior of the analytics component across all games). To place the analytics setup in a bootstrap build that executes before the game build and enable the provision of early analytics (on build loading and execution times or user acquisition).

2 - Timestep has been stripped off native components and building for native was therefore impossible. Native build was unmaintainable due to the lack of test suite or product in development. More people need to be able to learn and modify the build process and the added logic of the native builder was an obstacle to a quick understanding of the code base.

Changes

1

2 - Straightforward removal of all the files located in src/build/targets/native as well as code logics related to the native build in other files located in the src directory.

Lead for future improvements

Related tasks

@yofreke @rogueSkib @bengarney @fairfieldt @RhettAtBlack for review

bchevalier commented 7 years ago

@yofreke logic relating to isLiveEdit has been removed.

bengarney commented 7 years ago

At a high level I really like this. Less code is less maintenance and mental overhead. I like this but want to make sure we have a smooth merge. Is this tested against the games?

bengarney commented 7 years ago

Oh - is package-lock.json intentionally included?

bchevalier commented 7 years ago

@bengarney I intentionally included it because I thought that was the purpose of package-lock.json. Should I remove it from the PR?

bchevalier commented 7 years ago

@bengarney This is tested against Everwing. It can be tested against other games too.

bengarney commented 7 years ago

I defer to @yofreke on this.

A quick pass confirming other games are OK is good. Don't need to go super deep if EW is solid.

bchevalier commented 7 years ago

@bengarney actually I spoke too fast, to test other games would require some refactoring of their analytics, so not so easy. The idea is that they would have some refactoring to do when adopting the new build system.

bengarney commented 7 years ago

How long would it take to do that? If it's less than a day of work it's probably worth it to get extra validation of these changes. If it's more we may just want to leave them on an older branch for a while.

bchevalier commented 7 years ago

Actually I would rather have this PR validated (even if not merged), so that I can move on and include timestep in it. It will help facilitate refactoring the games to make them compatible (because some changes might be required in both timestep and devkit-core).

bengarney commented 7 years ago

Let's consider this solid enough to move on. You can do add'l PRs against this PR so we can digest smaller changes as we go along.