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

Refactor msp to modules #3214

Merged
merged 3 commits into from Jan 14, 2023

Conversation

chmelevskij
Copy link
Member

@chmelevskij chmelevskij commented Jan 5, 2023

Refactors all of the configurator files to use ESM modules.

This doesn't fix any circular dependencies, that will have to come as different PR to reduce amount of changes.

@chmelevskij chmelevskij force-pushed the refactor-msp-to-modules branch 5 times, most recently from fc5e62b to 2359280 Compare January 9, 2023 08:29
Comment on lines -218 to +224
GUI.log(i18n.getMessage('infoVersionOs', { operatingSystem: GUI.operating_system }));
GUI.log(i18n.getMessage('infoVersionConfigurator', { configuratorVersion: CONFIGURATOR.getDisplayVersion() }));
gui_log(i18n.getMessage('infoVersionOs', { operatingSystem: GUI.operating_system }));
gui_log(i18n.getMessage('infoVersionConfigurator', { configuratorVersion: CONFIGURATOR.getDisplayVersion() }));
Copy link
Member

@McGiverGim McGiverGim Jan 9, 2023

Choose a reason for hiding this comment

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

I like more the old way of GUI.log (use a class/object and call the method). This way is outdated/discouraged in modules world?

Copy link
Member Author

Choose a reason for hiding this comment

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

That thing was causing most of the circular dependencies.

It's not Java or C#, there it literally no need to use classes here. Classes only make sense of data is stateful, this log does literally nothing more than reach into dom and append message. There is no need for it to be in class.

Copy link
Member Author

Choose a reason for hiding this comment

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

GUI in general is used in soooo many places and needs splitting up. log should be it's own module/logic. But that's refactor for future. Now just fixing obvious issues.

Copy link
Member

Choose a reason for hiding this comment

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

Can we just call it log? Does it need gui_ ? That way you're just removing 'GUI.' and it's a suitable compromise.

Copy link
Member

Choose a reason for hiding this comment

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

It's the log that appears in the GUI, in the upper part. We have too the console log. It's good to differentiate them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Open for other naming. We also have logging tab and as @McGiverGim have console.log so its rather overloaded term.

@haslinghuis
Copy link
Member

usbDevices not defined while flashing.

@chmelevskij chmelevskij force-pushed the refactor-msp-to-modules branch 2 times, most recently from ce02e2d to 4f5b5be Compare January 10, 2023 08:06
@chmelevskij
Copy link
Member Author

@haslinghuis Fixed usbDevices export.

@chmelevskij chmelevskij force-pushed the refactor-msp-to-modules branch 2 times, most recently from 191a67b to 24582f7 Compare January 10, 2023 10:47
@haslinghuis
Copy link
Member

haslinghuis commented Jan 10, 2023

@chmelevskij thanks, as far I could test everything seems to work

Please fix yarn test

Tried to work it out but imports get 404. So might be a rollup thing.

@haslinghuis
Copy link
Member

Upon further testing found (vtx tab):

Error in event handler: ReferenceError: VtxDeviceStatus is not defined
    at Object._getDeviceStatusClass (<URL>)
    at Object.createVtxDeviceStatus (<URL>)
    at MspHelper.process_data (<URL>)
    at <URL>
    at Array.forEach (<anonymous>)
    at Object.notify (<URL>)
    at Object._dispatch_message (<URL>)
    at Object.read (<URL>)
    at read_serial (<URL>)
main.html#:1 Error in event handler: ReferenceError: VtxDeviceStatus is not defined
    at Object._getDeviceStatusClass (chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/main.js:853:13)
    at Object.createVtxDeviceStatus (chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/main.js:835:43)
    at MspHelper.process_data (chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/main.js:1102:69)
    at chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/msp.js:233:13
    at Array.forEach (<anonymous>)
    at Object.notify (chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/msp.js:232:24)
    at Object._dispatch_message (chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/msp.js:223:14)
    at Object.read (chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/msp.js:202:22)
    at read_serial (chrome-extension://ajdoecjhplhoahggfmgflfnlechcljoc/js/main.js:4625:13)

@chmelevskij chmelevskij force-pushed the refactor-msp-to-modules branch 3 times, most recently from 037806f to ee7721b Compare January 14, 2023 17:38
@github-actions

This comment has been minimized.

@chmelevskij chmelevskij marked this pull request as ready for review January 14, 2023 20:56
@chmelevskij
Copy link
Member Author

@haslinghuis I think I've fixed the VtxDeviceStatus issue

@sonarcloud
Copy link

sonarcloud bot commented Jan 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
0.4% 0.4% Duplication

@haslinghuis
Copy link
Member

haslinghuis commented Jan 14, 2023

Testing now :)

Yes vtxDeviceStatus looks like to be solved now !!!

🎉 🥳 ❤️

We should merge this soon.

@github-actions
Copy link
Contributor

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
Betaflight-Configurator-Linux
Betaflight-Configurator-macOS
Betaflight-Configurator-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

@haslinghuis haslinghuis merged commit c086395 into betaflight:master Jan 14, 2023
Finalizing Firmware 4.4 Release automation moved this from In progress to Done Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants