Closed PeteDuncanson closed 3 years ago
nathanwoulfe
commented
5 years ago Why not just straight to components? It's an easier conversion (IMO) and if that's where things should eventually be headed (and it is, again in IMO), just do it once now. Minimizes two-way binding which is potentially a great perf gain.
I just really like components.
Shazwazza
commented
5 years ago agree components are much nicer
nathanwoulfe
commented
5 years ago I'm happy to do a couple as an example. Will start with this guy, ContentAppContentController
Actually, probably more sensible to look at property editors first - main editor setup is a much bigger beast
PeteDuncanson
commented
5 years ago Suits me, kind of wanted to do it in steps but yeah, while there we just convert directly to components if you like. Get isolated scopes too WTF! But I worry it might all be a bit of a scope soup mess when you get to the bigger stuff.
I've looked into two so far and ran into issues with both!
and the boolean property editor, can't see for the life of me where it is actually used/referenced which is making me think it might not even be used. Its a damn hard code base to get your head around in its current state, need to fix this so people can jump in and help and be more productive.
Shazwazza
commented
5 years ago We should also start converting most directives to components (not all directives can be made components, like ones that are attributes, but most can move). We've done quite a few of those during the v8 build.
Property editors aren't referenced in angular, they are referenced from ... property editors :P It's from the TrueFalsePropertyEditor in c#. Property editors are the worst - and unfortunately we're stuck with this. They should all be components but they are just views + controllers and they must remain like that, though you could put a component inside the view. Property editors are loaded with ng-include, we can't change this now ... well, maybe with some clever engineering we might be able to support an old and a new way but ideally we'd load in components/directives dynamically with a bit of $compile magic (i.e. https://www.codelord.net/2015/05/19/angularjs-dynamically-loading-directives/) ... but that's all another story.
nathanwoulfe
commented
5 years ago Maybe the best approach is to find a section of the backoffice that can be done top-to-bottom, as a best-practice example? The content tree, for example.
PeteDuncanson
commented
5 years ago Yep. I’m all over that idea of an example section. Thanks for the info on prop editors. Had me pulling my hair out last night.
When we did a big upgrade in React last year we had this spreadsheet that listed every file, where it came from, where it should go, yes/no fields for stuff that should be done or checked. Sounds old school but it Made for a really good way to manage 250+ components getting messed with sanely. Happy to spend some time spinning something like that up for this if it would help?
Sent from my iPhone
On 7 Mar 2019, at 08:47, Nathan Woulfe notifications@github.com<mailto:notifications@github.com> wrote:
Maybe the best approach is to find a section of the backoffice that can be done top-to-bottom, as a best-practice example? The content tree, for example.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/umbraco/Umbraco-CMS/issues/4875#issuecomment-470437843, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABmNXn2JSzXwaHqtmPRJiGC2eGvYWMm8ks5vUNIcgaJpZM4bfOec.
PeteDuncanson
commented
5 years ago Finding more and more bits that are "quirky". Up front, I'm not judging here with my choice of words, its a huge code base grown over many years with not enough time, I get it and all these issues are from someone wanting to give it some time and try to get it a little closer to "oooh thats nice" territory. Right with that said...
I found this one today:
Its raises a lot of questions on style. This might need to be a separate issue maybe so we can get more eyes on it? But some bits I noticed that we would need to agree on so if we did this clean up work we have a "standard" of sorts. This is top down as I go through the file and not in any priority order, some is petty others are bigger issues:
Do note that because of JavaScript’s less-than-awesome handling of the this keyword, we now have to take extra care to access it correctly. As opposed to $scope,this can’t be used reliably inside controller functions.
If you’re using ES5, this likely means you should declare
var self = thisat the top of your controller and use that instead ofthis.If you’ve already started using some ES6 features, arrow functions(e.g. () => {}) can be used for making
thiswork properly in most scenarios.
That said if having "self" var makes using "this" easier for everyone I'm happy to keep it, again this would probably be called "vm" from what I can see through out the code base.
Phew, lots going on there, we don't need to do it all but just some of the things we need to think about while we go through this clean up stuff and some choices to be made.
Input welcome as ever :)
PeteDuncanson
commented
5 years ago Query, is there an AngularJS code standard at HQ somewhere that we could follow?
nathanwoulfe
commented
5 years ago Style is a big issue and needs to be consistent. I think we'll agree on that.
Personally, I prefer to avoid vm = this. Use ES6 fat arrows and get lexical this, which cleans things up a bit more. Similarly I prefer to simply use $ctrl rather than redefining it, it's one less boilerplate step to remember.
I tend to define the controller as a separate function and reference it in the component declaration. Why? It makes the file easier to read since it reduces nesting. Petty, probably. I do the same with the component object too - it's not inline in the angular registration, it's a separate object which I reference.
I agree with lifecycle hooks - not by reference. this.$onInit = () => {} every damn time. Defined last in the controller function.
Shouldn't need $scope or $watch at all. Can use $onChange hook to catch changes to any bound values passed in, and if we need to pass up through the component tree, there is an output API (which I can't remember right now - it's 5:20am and I'm between sets at the gym). Scope should be abolished, with components sent the appropriate values by their ancestor controller (which ultimately could be a component itself).
Should definitely be named and registered correctly. Naming things is hard, but we could namespace by type then feature, or vice versa. umbraco.components.editors.boolean for example.
My 2c.
Shazwazza
commented
5 years ago Oh my this is a bit of an overwhelming amount of info :P ... and perhaps a style discussion needs to happen elsewhere, or we can change the title of this task.
Consistency is important, style is important, etc... and we all have our likes/dislikes. vm was a standard adopted by mads, we didn't have es6 before so yes probably made things easier and it's throughout the codebase now. I personally think spending the time to change all of this over to a new format is wasted time rather than just continuing to be consistent. Do we really want to change coding style in an already existing massive project? If we do then we'll have even more inconsistencies since I can't really see changing everything over to a new coding style, that would be probably thousands of files to change and review. If we want to proceed with changing the coding style, then we'd need a plan for doing that.
Query, is there an AngularJS code standard at HQ somewhere that we could follow?
No sorry, it really comes down to using what is there - we can collectively start defining something.
As for inline functions... why? defining functions as functions makes things easier to read, one things for certain we want to avoid nesting as much as possible, it makes things very difficult to read.
umbraco.directives and the folder naming conventions is a PITA and again, just comes down to time. This is a problem with the gulp build which looks for certain file naming patterns. I also really dislike that all directives/components have a different folder for the .js and .html files. We've wanted to fix this for a very long time but don't have time... it would be quite a big job but perhaps the first step is to define the correct file/folder/module naming conventions and start using it? Then migrate slowly.
We've a postLink function which creates a $watcher on one of its children.....
yes .. there are reasons for things you know ;) When dealing with "forms" in angular, you can't just deal with a component tree type thing, you don't pass in a form, a component's html declares a form and the way angular works is it attaches it to the scope/vm/whatever. If you need to monitor changes like validation on ngFormController, you need a $watch, happy to be proven wrong though.
nathanwoulfe
commented
5 years ago FWIW, the current gulp task does some transpiling magic - arrow functions are converted to es5 functions, and lexical this is converted to _this. Is gulp the best tool for the job?
Maybe it is a mixed approach - existing style remains, but new work follows current best practice.
I guess too there needs to be consideration for future major versions. Will the backoffice always be AngularJs 1.7.5? Are there benefits to be had by adopting conventions more closely aligned with other frameworks? Not looking for answers, just posing questions.
Shazwazza
commented
5 years ago Maybe it is a mixed approach - existing style remains, but new work follows current best practice.
This can be fine by me so long as we document this somewhere and make it clear how we want to work and maybe over time can port things over to how they should be.
IMO I wouldn't worry too much about aligning our coding style with other frameworks. I think we stick with what works for us with this solution. If we ever move to something beyond angularjs, it will be a mammoth project - like enormous, we're talking beyond the effort of moving to aspnet core. And in that case we'll be rewriting almost everything. Though this is a good point to make with regards to not having inline functions and using function declarations ... normal functions can generally be moved as they are anywhere.
Note - the back office will remain on angularjs and as far as writing this they don't plan to go beyond 1.7.x but they continue to output new versions and with new features so we'll keep up with the latest angularjs versions moving forward.
PeteDuncanson
commented
5 years ago I've moved the "style guide" conversation over to another issue (https://github.com/umbraco/Umbraco-CMS/issues/4913).
I think there is a lot to be said for getting everything "componentised" up as best we can as it should make swapping over to another framework int he future easier to rationalise about. With some work the components can end up looking pretty much like standard Es6 classes (or not far off). That was sort of my thoughts with this whole issue/job, step by step get us closer to that sort of setup and end up with a tidier code base in the meantime. It is not going away anytime soon so investing some time on it now and reap the rewards in easier bug fixes and feature developments and hopefully more PR's.
Sounds like Shannon knows the angular stuff thats already there better than anyone now (Mads and Per are no longer around to ask) so any chance we could have a Zoom session and a bit of a tour of some of the quirky bits (like why the login page is as it is) and which areas we could dive into and get componented up? Then we can document that (even record it too) to help guide ourselves and others on what to do and why some of the stuff is done the way it is.
BTW I'm still rusty on my AngularJS (my limited knowledge comes from package development not back office app stuff) and I've been off in React land so some of my knowledge mappings in my head might be wrong (like the $dirty stuff). Please feel free to correct me, I'm like a sponge at the minute and wanting to load up my head so I can get this right :)
Shazwazza
commented
5 years ago Sorry for the delay. Would be happy to do a zoom call, perhaps next week or something (this week is no good) but would like to make sure we have some clear questions to answer since we'll need to keep this less than an hour. Thur apr 4 or thur apr 11 between 9am and noon DK time would work for me.
umbrabot
commented
3 years ago Hiya @PeteDuncanson,
Just wanted to let you know that we noticed that this issue got a bit stale and might not be relevant any more.
We will close this issue for now but we're happy to open it up again if you think it's still relevant (for example: it's a feature request that's not yet implemented, or it's a bug that's not yet been fixed).
To open it this issue up again, you can write @umbrabot still relevant in a new comment as the first line. It would be super helpful for us if on the next line you could let us know why you think it's still relevant.
For example:
@umbrabot still relevant This bug can still be reproduced in version
x.y.z
This will reopen the issue in the next few hours.
Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:
Quick search through the source code found over 277 instances of stand alone controllers being referenced. Ideally as part of making the most of the recent AngularJS upgrade we should move these into their own directives to make their usage clearer, ensure they have an isolated scope and that we only pass into them what they need. It should end up with a nicer work place too.
The plan here is to wrap all the standalone controllers in a directive first and then later we can come back and convert some of them to components later for yet more gains.
This is another step in the process of making the AngularJS setup great again, little by little.
How to do it:
Do a search for:
https://github.com/umbraco/Umbraco-CMS/search?q=ng-controller%3D%22&unscoped_q=ng-controller%3D%22
Pick a controller you fancy converting from the search results. Often the controller is referenced in the template in the search results and they normally (well so far anyway) live next to each other on the file system. So click on a result and then go find that in the source code and you will find the controller next to it.
Its worth while doing a separate search for the controllers name on Github to see if its used in multiple places, so far I've not found any but worth while double checking. If its not used twice then we need to wrap it in either a directive or a component.
Probably best to add an example next but I haven't done one yet, guess this issue will be a holding pen until I do so. Seeing some code and how its done should make it clearer and then hopefully more can jump in and help.