-
-
Couldn't load subscription status.
- Fork 1k
Add Vue js into the codebase #2182
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
Conversation
|
Hi @chmelevskij ! Thanks for this ! In the latest months we have discussed several times to "modernize" the Configurator framework. The problem is that we not decide to which framework we must to go. I don't have experience in any of the modern frameworks, so I'm not able to argue in one direction or other. Here there is another POC of adding React for example: #1769 made by @ademuk some months ago. @mikeller I've seen in the forums a growing interest in the latest weeks to migrate to modern frameworks. Maybe is the moment to choose one, that can be used incrementally. Have you some opinion about what must be our final framework? I'm sure that any of you @chmelevskij @ademuk @TomChe @DusKing1 @pevecyan @haslinghuis @freshollie (with his complete test to rewrite it https://github.com/freshollie/fresh-configurator) and many others have more knowledge about this. Maybe you can explain why choose vue, react, other to help us decide? Feel free to invite anyone with knowledge in the theme to discuss in this thread. |
|
@McGiverGim I've put this up in the Slack for a poll (as you might have seen). I'm personally a full time I've seen the rewrites, the main issues with complete rewrite are:
So why Vue and not React?Some of the reasons choosing Vue ToolingAs you can see in this PR, vue has a modern module output, fact that we are using NWjs means we don't even have to have tooling at first. We don't need to explain to people how to build some special syntax. Reactivity systemAgain as you can see in my PR, Vue has really easy way to plug in model into current code. With react it doesn't come with that straight out of the box, a lot of time would be spend on arguing what to use etc. Future, stylesIt's possible to gradually introduce the tooling and allow single file components, what solves the scoped css issue, again no need for choosing 3rd party solution. Why not Electron?NWjs is perfect for this usecase, it has direct access to serial and also supports chromeOS. MigrationI've seen people talking about fancy rewrites and etc. IMO this project will benefit from those ideas, but it would take ages to do that. With this approach it is possible to focus on the UI part first. Gradually migrate to how just the representaion is handled. No changes to the code responsible of the logic. Once that is in place then it would be time to rearhictect the app, split it into packages or whatever you want. But again doing this gradually. That way project would have verification after every tiny change. I'll sit down and write proper RFC and create it in Issues once I get a chance. What about typescript?Once again, my full time job. So why not to use it? Extra build tooling and familiarity for other developers. Typescript is dead easy to drop in if you have build pipeline. So once vue would use some tooling, typescript would follow. |
|
As you have seen in my comment, we don't want a fully rewrite because we don't have enough developers to do that. That is something we have clear. The code needs it, specially the html and css part, but we don't have the power to do it. The incremental approach is needed. Let's see if we can move to components with this incremental approach and see if more developers want to help if we have a more modern framework. I've seen the comparison WARNING: FROM THE VUE PAGE if someone wants to look at it: https://vuejs.org/v2/guide/comparison.html If you are a React developer and you think Vue can be better, is a very valuable opinion. Let's see what others think. Thanks for this again. As I say, we are for the migration, incremental, only need to decide the destination :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only some comments, I've played a little with the vue tutorial and I have some doubts. Maybe this review with questions help to understand what is the final idea.
src/vue/init.js
Outdated
| import vueI18n from "./i18n.js"; | ||
|
|
||
| const logoModel = { | ||
| CONFIGURATOR: CONFIGURATOR, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this object logoModel is only for the logo component, I think is better to pass only the minimum data that it will need. It makes it easier to understand and easier if we use the minimum variables and not complex objects that we don't use. Maybe in a future we want more, but until then pass the complete CONFIGURATOR object seems too much.
So I think is better to pass only a versionConfigurator (or configuratorVersion if you apply my other suggestion). Something like:
configuratorVersion: CONFIGURATOR.version,BUT I've tried and it does not get attached to the variable and I think it will not react automatically when we change the CONFIGURATOR.version when loading. I'm very newbie to this. This can be achieved easy in some way o do we need to refresh the configuratorVersion in the same way we do for the other model variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will grab a look, I think it should be fine with smaller object. This was just the shortest path to get what I want 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok have a try and I see what's happening. So Vue uses some js getter/setter magic to make objects reactive once you pass them to the Vue instance.
Since version is primitive it's passed by value so Vue has no idea that CONFIGURATOR has changed.
In general yes, components should know least amount possible. There are at least 2 ways approach this:
- Keep it as a complex object. Once more or even all components are migrated more components will need access to
CONFIGURATOR. Also it's important to rememberlogocomponent receives primitive strings. Only the rootprops: [ #logoof it receivesbetaflight-configurator/src/main.html
Line 158 in 210cc16
<div id="logo"> CONFIGURATOR, once more components are implemented single root will have more and more components which would need access to those values so this would make more sense. - other option would be to update the model in a similar way to
betaflight-configurator/src/js/main.js
Line 747 in 210cc16
window.vm.firmwareVersion = firmwareVersion;
There are other ways to achieve this of course. But I like the current one since it hooks up into current system quite easy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With that being said, I think it would be even better idea to pass some of the other global objects like FC.CONFIG. This way that would even remove this
betaflight-configurator/src/js/main.js
Line 745 in 210cc16
| function updateTopBarVersion(firmwareVersion, firmwareId, hardwareId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, this is something that needs to be decided. Not a problem for now. Let's see if some experienced Vue developer goes here and gives his opinion.
|
|
||
| i18next.on("initialized", () => { | ||
| vueI18n.setLocaleMessage("en", i18next.getDataByLanguage("en").messages); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introducing this vue i18n for translations will complicate things a little... we don't use the i18next library directly because we have first a chrome app and we use the chrome way for pass parameters in the messages file, was positional, with arrays, etc.
If the vue i18n give us benefits is not a problem, but maybe we need to modify some translations when we add it to the vue way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There exists vue i18next version as well. But I've chose this one because other one currently doesn't have the ESM compliant build.
The main reason to use this is to get translations like $t("versionLabelConfigurator.message") in the Vue templates. i18next seems to have decent event system implemented so it's quite easy to make them two talk. Eventually this would need the refactor of course. But I think that would be more beneficial to do once all of the UI is in Vue( or well any other framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I say is that precisely we use i18nex but with some "wrapper" to maintain compatibility with the old chrome way for some of the old messages that use it:
betaflight-configurator/src/js/localization.js
Lines 62 to 86 in 59076c0
| i18n.getMessage = function(messageID, parameters) { | |
| let translatedString; | |
| // Option 1, no parameters or Object as parameters (i18Next type parameters) | |
| if ((parameters === undefined) || ((parameters.constructor !== Array) && (parameters instanceof Object))) { | |
| translatedString = i18next.t(`${messageID}.message`, parameters); | |
| // Option 2: parameters as $1, $2, etc. | |
| // (deprecated, from the old Chrome i18n | |
| } else { | |
| translatedString = i18next.t(`${messageID}.message`); | |
| let parametersArray = parameters; | |
| if (parametersArray.constructor !== Array) { | |
| parametersArray = [parameters]; | |
| } | |
| parametersArray.forEach(function(element, index) { | |
| translatedString = translatedString.replace(`$${(index + 1)}`, element); | |
| }); | |
| } | |
| return translatedString; | |
| }; |
Not a big problem, but we need to take it into account.
68c812d to
e064761
Compare
|
@McGiverGim I've updated the code with more stuff ported to Vue. One improvement thought it's possible to sort of mix and match vue with existing code. As you see now the whole app is conatiner for Vue. This wrapping could be shrunk down just to widgets or even single instances like I've previously had or keep it on the main content and just gradually adding Vue components. As you can see even old jquery code still works inside of the Vue instance. |
ae8a4e9 to
585d0c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the code, now it explains better how it can work in a future.
The only problem is that now the PR is bigger.
- I liked the old because it was very simple to show the POC how to integrate. But I suppose it does not demonstrate the power of the reactive thing. We were using a method like in the old code.
- I like the new because it shows how it must work for real with more elements in the UI.
I think remember we have some elements that need some kind of conversion when going from the MSP value (FC variable) to the UI and the inverse. I suppose all of this can be done easily. But this and a lot of other things is better to wait for the final adoption.
But let's go part by part. Let's see if we choose Vue or React and then I hope you can help with some example of more things.
I've talked with @mikeller and I think we agree that is a good moment to take the decision.
|
Yep, it's quite big, but definitely shows the beauty of Vue low level entry barrier for adoption. Especially the fact that currently a most of the stuff had initial values, so that made it super easy to add. Like I've mentioned before, I do spend my working hours on React code, and just don't see it being that easy to adopt. A lot of decisions would have to be made about things like, how to connect the state? How to style, how to add build tooling? Could try implementing the same parts in react, but not really keen 🤣 Defo will take longer to setup |
|
My personal opinion is that React was made to suit Facebook's needs, specifically to make their desktop notifications work. This was clearly stated in the initial demonstration when React was revealed. I do agree that the initial design constraints required to employ React in a project are too far stretching. |
|
I mean I love react, it's easy to reason about because of unidirectional data flow and all. BUT, it was made by a massive giant having a full time team working on it. Everywhere I've worked I'd personally spend reasonable amount of time configuring it, choosing tools to go with it and so on. This was a paid job. Also you can design Vue application to have this unidirectional data flow as well. It's no re-inforced though, but that gives team ability to decide. No one here is really paid to do this. So I think the fact that vue has router, state managment, styling and tooling coming from the same team means there is less time bike shedding, should we use this css-in-js or that one? Should we use mobx or redux, or . |
|
Unidirectional data flow is a concept and as such it is not tied to any language or framework or whatever. For this reason I don't think it is a real advantage of React over something else. EDIT: Just drop RxJs into any project and basically you're done :) |
It's not, sure, but in react it's pretty much the only way to deal with data 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add space
|
Anyway, I think Vue (with TS) is a valid way to go forward. Imho only Aurelio is competitive |
|
I think TS would be cool as well. And most frameworks support it so it's could be separate PR |
|
@McGiverGim @mikeller Is there any progress in thinking about this? Is there any more code, examples or questions? |
|
No, all is clear. I've talked about it with mikeller and we will announce the final decision soon 😁 |
|
@chmelevskij: Yes, looking into this, along with other potential options like React. One thing that will have to happen first is that we will have to drop support for the Chrome web app version of configurator, as Vue would need to be browserified to run in it. |
|
@mikeller Sorry, I don't understand what you mean with:
So you want to drop Chrome app support, because Vue needs to be browserified? Doesn't sound right... Modern browsers already support In general, I think when it comes to modules/bundling the aim should be to work towards using native esm modules. Do you need an example in React then? Could try to ramp one up for the same codebase, but it will be a bit more involved, because, well, it's react 😅 so would need another week or two. But if that would help with decision happy to do it 😉 |
|
What just worries me is that this spreadsheet been created almost a year ago now. By this point it doesn't really matter that much which one you pick. But it's about time to choose one 😅 Otherwise there be more and more forks and rewrites seeing how the main project is stuck in choosing the best framework. |
|
@chmelevskij no, we don't need a React POC, we have another one that I linked in the conversation. What I think @mikeller wanted to say is that the decision has been slow because he has being comparing both. And yes, we want to remove the old chrome support. Some things like import do not work. We are talking about chrome apps, not that it do not work in chrome browser. You can test loading the dist folder as extension in chrome and it gives a lot of headaches to maintain at the same time that node.js. We want to get rid of it too in the process. This architecture decision follows a process, internal discussion, etc. please wait some time more. I want to have it available for hacktoberfest, is a good way to get some PR that use it, but I can't promise nothing... |
|
cool, sorry if I sound to pushy. Just putting out questions to see if I can be of any help 😊 One thing which I've mentioned before is that migration to some js framework shouldn't be that strongly tied to the re-architecture of the whole app. Also chrome extensions are not exception for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some little comments to the review too.
Another question: I've seen that there is an extension for Vue that can be installed in Chrome to help debugging... can this extension be installed in our node/nw system in some way? Will be very valuable to have tools that help with the code.
| updateTopBarVersion(); | ||
| // with Vue reactive system we don't need to call these, | ||
| // our view is reactive to model changes | ||
| // updateTopBarVersion(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't like to have commented code. I understand that for the POC is useful but please remove it before the final version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, more for the illustration of Vue 😉 put it so that I didn't forget to comment it.
In general. Will tidy up and split the commits if it comes to merging this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commentary remains, please remove it. I think is the latest to approve it (at least in my side).
src/js/serial_backend.js
Outdated
| $('span.cpu-load').text(''); | ||
| // TODO: vue would need to check the version as well somehow | ||
| // if (semver.gte(FC.CONFIG.apiVersion, "1.20.0")) | ||
| // $('span.cpu-load').text(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only a reset. I don't now why it resets the contents of the CPU load based on the version. Maybe it is not needed in earlier versions, but it does not hurt neither. I suppose that now that they are reactive we reset them when we reset the FC object. So we only need to be sure that the FC object is being reset when before we show the fields.
| filters: { | ||
| stripEnd(value) { | ||
| return value.replace(/\$1%/, ""); | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hack needs to be fixed in a better way:
- One solution is to "migrate" the messages phrases to be compatible. The only problem with this is that all the translators will need to change the translations too, and this is a big work.
- The "hack" must be only at one place. Now you are adding this to any component with the problem, this is not a good way.
- I'm not too sure what this replacement does exactly. Can you explain better what the problem is with one example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the "best" solution would be to just use appropriate formatter for i18nvue. Default one uses {token} syntax, but it's possible to just write custom one which would use the same syntax as i18nnext.
about multiple places. It's possible to register filter with single component or just the instance. Could easily pull this out if needed to just the status bar.
Messages had CPU load: $1% in them, this strips that end part so that you don't get CPU load: $1%20% etc. in status bar.
(more info about custom formatters)[https://kazupon.github.io/vue-i18n/guide/formatting.html#custom-formatting]
It should be possible, I've done similar thing with In general I'm thinking about few improvements for DX:
So yeah, there are some thoughts on how to improve DX in general 🙃 |
Yeah, not quite: |
Do you have a repro branch by any chance or instructions on how to reproduce? These errors are not module related, it's error about how Vue templates are rendered in the extension. It's possible to enable the Another solution which would work as well I think is to use Vue compiler at build time. |
|
Apart that, Vue seems to work well with Cordova |
|
Thanks @WalcoFPV for your help with this. Glad to know that Vue works ok with Cordova. |
510ad7d to
6c7b37a
Compare
|
Can somebody with android test this out? @WalcoFPV ? |
There is an issue with the logo of the sidemenu |
6c7b37a to
c6d7a58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment about string templating. Apart from that the code looks good to me.
What I would like to discuss is the proposed way forward with respect to the folder structure. I don't think that what we currently have (html hangs directly off /src, everything else is one layer down from it) isn't a good way of doing things. So I think we should be considering if there is a better approach than adding another type of source in its own subdirectory.
Re casing, my personal preference is for PascalCase over kebab-case. But that's just that, a preference.
From practical experience, most modern file systems seem to be able to handle case sensitive file names sufficiently well, so I don't see a problem there.
However, the case insensitivity of html is an issue here - to me, introcuding a mapping between PascalCase entities in code and kebab-case equivalents in html is not a good approach, as it makes it harder for developers looking at code to find all occurrences of a particular entity, so I think using consistently using kebab-case for entities that are used in html templates is a better approach.
The obvious other thing that we need to look into is to make this work for the android version.
c6d7a58 to
960760d
Compare
Fixed that (I think) just need a second pair of eyes best if with a real device to verify it. File structure and casing issues are kind of related.
Similar to the casing, most of the structure would gradually move from nested |
I think the recommendations of Vue is to do exactly that, PascalCase always except for html, so I suppose the developers do it in this way and is how the developers will expect to find it. If they recommend this way I suppose it has not produced any problem in the latest years. |
Where do you think the pure .js componentry for the backend of the application should live in your proposal? |
|
I imagine a flatter Currently |
|
@chmelevskij: Makes sense. But not sure if we want to stick these components into a |
Logos are fixed on Android ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Only one minor change in this review
- The old change request of the older review about removing a comment that remains in the code.
For the rest, to me is ok. We can always change a thousand things, but we need to start with something.
About moving it to Components it seems the Vue Style Guide use components in lower case.
960760d to
d6cee38
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chmelevskij to me this is approved! Thanks for your patience all this time with this PR.
| "default_locale": "en", | ||
| "scripts": { | ||
| "start": "gulp debug", | ||
| "start": "NODE_ENV=development gulp debug", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks yarn start on windows 10 ... @chmelevskij pls can you check whats wrong
The "NODE_ENV" command is either misspelled or could not be found.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, don't have access to windows. How do you set environment variables in windows usually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set VAR=VALUE
| "start": "NODE_ENV=development gulp debug", | ||
| "gulp": "gulp", | ||
| "release": "gulp release", | ||
| "release": "NODE_ENV=production gulp release", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try this PR #2226
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
working again ... thx @atomgomba
|
@TheIsotopes will add https://www.npmjs.com/package/cross-env tonight 😉 Sorry about that |
|
@chmelevskij there is an alternative package and it seems to provide a more readable syntax to solve this: |


POC for using view without even build tool and making it talk with the rest of the code. Idea is to figure out best way for gradual refactoring.