Skip to content

Conversation

@chmelevskij
Copy link
Member

@McGiverGim I've moved localisation.js to modules. However there is one small caveat is because it's in NWJS.

NWJS exposes require in the UI and allows importing native modules that way. But, it doesn't allow consuming in built node modules through ESM. So, import fs from 'fs' won't work.

I've added a solution where I just replace the import as string, but it's quite unstable. Haven't tried it but if another part of the app is migrated to import/export it will brake I think.

It is quite a unique issue, since usually you either run in browser or in node, this one is a mix.

I could potentially write a rollup plugin for this if it pops up quite often 🤔

@McGiverGim
Copy link
Member

I don't understand exactly the implications of this, I need to look at it slowly.
The objective was to fix the Android build. I need to test it.

@chmelevskij
Copy link
Member Author

Android is using cordova. Which is just webview in the app. It doesn't have access to things like filesystem in a way NWjs/node has. So using filesystem loader won't work 😕

I think previous solution with XHR request would be needed. Maybe just change it so that messages with initial load are baked in somewhere and switchign the language would load the new ones async

@McGiverGim
Copy link
Member

We removed chrome to have only one coding for all, it seems it will be impossible! Or at least we need to go back and return to old code...
Our code waits to load with a callback, but Vue is not doing it because it does not execute the init method.

@McGiverGim
Copy link
Member

One thing more @WalcoFPV said it has included the nw apis to Cordova. So maybe the fs will work but I don't know if it needs some hack to do it.

@WalcoFPV
Copy link
Contributor

One thing more @WalcoFPV said it has included the nw apis to Cordova. So maybe the fs will work but I don't know if it needs some hack to do it.

I included the Chrome apis, not the nw apis. But the node modules should work with Cordova as well. I think, we must separate all the functionnalities depending on Cordova apis and Nwjs apis and create our proper apis. For example, creating an object apis or native with methods like native.serial, native.clipboard, native.filesystem, ... defined for both Cordova and Nwjs with their proper apis, but we will have only one api in the "common" part of the code for Cordova and Nwjs. All the things which should be done differently between Cordova and Nwjs should be done like that I think.

This will not fix the issue with the require function not functioning with Cordova but it will allows us to replace old Chrome apis with Cordova and Nwjs apis in an easy way.

@McGiverGim
Copy link
Member

I think this code is a good case to try to get to a final solution. We are using one module as i18next backend that internally uses the node fs api.
Do you think there is an easy solution for this?
We need to search one way to make it work for both systems without hurting too much the developers.

@mikeller
Copy link
Member

I think using an API wrapper is the right approach here. Luckily we now only need two implementations of it, for NW.js and Cordova, and not one for Chrome Web apps as well.
Instead of writing our own wrapper, I think we should first see if something suitable is already available, like https://github.com/evanshortiss/html5-fs or https://github.com/LockateMe/cordova-plugin-file-node-like.

@McGiverGim
Copy link
Member

I'm not too sure if I'm understanding what kind of wrapper are we talking about:

  1. The best approach is to see if there is some wrapper, created by someone else, that we can add and produce a totally transparent use of the Node api in Cordova (at least the fs api, that is the one most used). In this way we can use Node plugins and Node api directly in our code.
  2. The second one is to find a wrapper over the filesystem that works for both, Node and Api, but I suppose this will not replace the Node API so the plugins that use the Node API will not work in Cordova. We will need to use other plugins.
  3. The wrapper that @WalcoFPV comments I'm not too sure if is the same than 1 or 2, but created by us.

@WalcoFPV you are the only one here with enough knowledge about Cordova. Do you know if 1 is possible? If true, have you time to try to make a working code, starting for example with this PR? Is a good example of what we want. If not possible then we need to pass to option 2, and remove the use of Node plugins.

I think is good that we can use modules, with the approach of this PR or another one if someone knows a better approach, using import if require is not supported by Cordova.

@mikeller
Copy link
Member

@McGiverGim: I guess all of the above. :-)

Also, thanks to the open nature of JavaScript, it is possible to polyfill functionality, and have wrappers that transparently replace native functionality, even if it is called from within modules. But having some sort of abstraction layer, and preferrable one not implemented and maintained by us, is the way to go in my opinion.

@mikeller
Copy link
Member

I think this is outdated now since we have reverted the change to the fs backend for i18n. What should we do with it?

@McGiverGim
Copy link
Member

I'm not too sure, this is s good example that we can need at some moment, but true, at this moment and for this scenario is not needed.

@chmelevskij
Copy link
Member Author

This wasn't really tied to the fs. I'll adapt this one to the current master and will push it this weekend.

It will setup general steps on how to migrate other parts of the app to modules.

@chmelevskij chmelevskij force-pushed the use-modules-in-locales branch from a148a9a to 45f6a40 Compare November 2, 2020 20:58
@@ -1,4 +1,4 @@
'use strict';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modules are in strict mode by default

@@ -1,10 +1,12 @@
'use strict';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strict mode is default mode when using modules.

@chmelevskij
Copy link
Member Author

Adjusted to use xhr backend now and some cordova changes. Also remove some of the script tags, since rollup handles the bundling.

@WalcoFPV or someone with access to android, it would be good to verify that it works. I've only run some smoke tests on android emulator.

@chmelevskij chmelevskij force-pushed the use-modules-in-locales branch from 45f6a40 to fb74de3 Compare November 2, 2020 21:01
McGiverGim
McGiverGim previously approved these changes Nov 3, 2020
Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works in the emulator it must work in the real device. At least a device with the same configuration/versions that the emulator. We can have problem with different devices, but this will happen we test it in an Android or not. Until merged and used by others with different devices we will never be sure, but this will happen with any change we do.

To me is ok, I think is a demo of how to use modules with import in our "new" Configurator. I think is valuable and we can merge it.

@chmelevskij
Copy link
Member Author

Yep, I think this pretty much covers most of the cases.

There will be things likeimport '../js/localization.js'; which have side effects of attaching things to window since a lot of stuff relies on global singletons to work. But eventually we should be able to get rid of these things.

@McGiverGim
Copy link
Member

There will be things likeimport '../js/localization.js'; which have side effects of attaching things to window since a lot of stuff relies on global singletons to work. But eventually we should be able to get rid of these things.

Yes, that is probably the worst, because the i18n.getMessage() is widely used across the application.

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chmelevskij one thing more, the tests are failing. I didn't remember the tests... I had the same problem when I tried to do this...
They are configured to be executed in a Chrome browser...
Can you check at them?

@chmelevskij chmelevskij force-pushed the use-modules-in-locales branch from 20105c3 to 580213f Compare November 3, 2020 20:33
@chmelevskij chmelevskij force-pushed the use-modules-in-locales branch from 580213f to 73172bd Compare November 3, 2020 20:49
McGiverGim
McGiverGim previously approved these changes Nov 3, 2020
Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! Thanks for fixing this!

Co-authored-by: Míguel Ángel Mulero Martínez <mcgivergim@gmail.com>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 4, 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.0% 0.0% Duplication

@mikeller mikeller added this to the 10.8.0 milestone Nov 4, 2020
@mikeller mikeller merged commit 71586c9 into betaflight:master Nov 4, 2020
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.

4 participants