-
Notifications
You must be signed in to change notification settings - Fork 16
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
[DO NOT MERGE] feat: run node 16 on device #1104
base: develop
Are you sure you want to change the base?
Conversation
nodejs-mobile dropped support for x86 architecture (for now): nodejs-mobile/nodejs-mobile-react-native#34 (comment) https://github.com/nodejs-mobile/nodejs-mobile/blob/main/doc_mobile/CHANGELOG.md#2023-01-26-version-16170
looks like backend tests are failing due to upgrading Node version from 12 to 16. seems to be related to different streams behavior.
seems like the main reason is the result of this line:
In node 12,
I'm noticing that this listener in
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": 1100/upgrade-nodejs-mobile-rn...ac/node-16-upgrade-backend-tests cc @gmaclennan |
not going to merge this but keeping it as a draft for the foreseeable future. refer to #1100 (comment) for more details |
Closes #1100
Notes:
x86
architecture entirely (for now).nodejs-mobile
v16.17.0 dropped support for this, although it may reintroduce it later. according to our download statistics, not many devices using mapeo have this architecture (~7 active installations as of this week) but this is a notable change that's being introduced, so should be approached carefully.nodejs-mobile-react-native
and our project due to issues introduced in npm v7+. basically, this updated nmrn version attempts to workaround the npm issue but it specifically doesn't work forbetter-sqlite3
because of the way that project is structured (additional context here). as a result, we upgrade to node 16 but developers need to explicitly use npm v6 for now.npm_config_node_gyp
environment variable andnpm config set node-gyp
config don't work with npm 7 npm/run-script#23Due to the NDK version being upgraded, we need to update the docker image we use for Cirrus CI. work-in-progress PR is here: upgrade ndk for compat with latest nodejs-mobile-react-native docker-android#3. Ideally we'd update the docker image and make sure this PR successfully runs on Cirrus before merging itUPDATE: done and updated this PR - looks like it worked!