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

Fix require modules with Cordova #2262

Merged
merged 2 commits into from Oct 28, 2020
Merged

Fix require modules with Cordova #2262

merged 2 commits into from Oct 28, 2020

Conversation

WalcoFPV
Copy link
Contributor

This fixes the importation of modules with require with Cordova (#2246)

@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.6% 0.6% Duplication

@mikeller
Copy link
Member

This was less involved to browserify than I expected. 😅

@McGiverGim
Copy link
Member

@WalcoFPV can you confirm that the Android build is not totally fixed with this? I suppose that the require error is fixed, but we were using it to load a node module that uses the fs, so I suppose that this has not fixed it. I'm right?

@WalcoFPV
Copy link
Contributor Author

@McGiverGim Yes node fs still doesn't work and also loading modules with import doesn't work as well.

@McGiverGim
Copy link
Member

With this PR pending the import does not work neither? #2255

I think we have two big "open" themes for Android support:

  1. We need to support modules in Android. This PR makes the require work, and the chore: ESM for localization #2255 maybe makes the import work (waiting for your confirmation).
  2. We need to find a node api wrapper (if exists) that works with Cordova, as commented in chore: ESM for localization #2255 . If we can't find such wrapper, we need to revert the PR Replace i18next http backend by filesystem sync backend #2245 and search for another solution. We will then need to limit to non node modules.

We need the answer to this two themes first and after that, in a future, we can recommend to use simple wrapper for common operations (filesystem, clipboard, etc.) that can help to maintain the Node/Cordova compatibility. We will need to be cautious with this type of inclusions because we are seeing that we can break Cordova fast...

@mikeller
Copy link
Member

@WalcoFPV, @McGiverGim: Should we merge this, or do we want to keep hacking away at a better solution for Cordova?

@McGiverGim
Copy link
Member

If we are going to accept the require way of adding modules (no restrict to import), I think we need to merge it. I will revert the fs backend module in the next days to make Android work again.

@mikeller mikeller merged commit 5309bdf into betaflight:master Oct 28, 2020
@WalcoFPV WalcoFPV deleted the cordova_require branch July 13, 2021 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants