digidem / mapeo-mobile

Monitor and document the world around you
GNU General Public License v3.0
95 stars 16 forks source link

[DO NOT MERGE] feat: run node 16 on device #1104

Open achou11 opened 1 year ago

achou11 commented 1 year ago

Closes #1100

Notes:

achou11 commented 1 year ago

looks like backend tests are failing due to upgrading Node version from 12 to 16. seems to be related to different streams behavior.

  1. for one of the beforeAfterStream tests, this test is failing:

https://github.com/digidem/mapeo-mobile/blob/95beddfef3e71ca3522f1fd2b6b87dcd53678e3d/src/backend/test/utils/beforeAfterStream.test.js#L344

seems like the main reason is the result of this line:

https://github.com/digidem/mapeo-mobile/blob/95beddfef3e71ca3522f1fd2b6b87dcd53678e3d/src/backend/upgrade-manager/utils.js#L361

In node 12, ok is true but in node 16, ok is false. seems like 16 actually has the behavior I would expect for this test's case.


  1. a couple of the upgrade-storage tests are failing:

https://github.com/digidem/mapeo-mobile/blob/95beddfef3e71ca3522f1fd2b6b87dcd53678e3d/src/backend/test/upgrade-storage.test.js#L193

https://github.com/digidem/mapeo-mobile/blob/95beddfef3e71ca3522f1fd2b6b87dcd53678e3d/src/backend/test/upgrade-storage.test.js#L261

I'm noticing that this listener in beforeAfterStream is not being triggered as expected in node 16, causing the tests to fail I guess:

https://github.com/digidem/mapeo-mobile/blob/95beddfef3e71ca3522f1fd2b6b87dcd53678e3d/src/backend/upgrade-manager/utils.js#L341


In both cases, I'm able to add a couple of lines to fix them, but they're very uninformed and while all of the backend tests pass as a result (locally), I still lack confidence.

Created a branch with these "fixes": https://github.com/digidem/mapeo-mobile/compare/1100/upgrade-nodejs-mobile-rn...ac/node-16-upgrade-backend-tests

cc @gmaclennan

achou11 commented 1 year ago

not going to merge this but keeping it as a draft for the foreseeable future. refer to https://github.com/digidem/mapeo-mobile/issues/1100#issuecomment-1593522398 for more details