V8: Agree on AngularJS coding style and standards moving forward

[PeteDuncanson]

There is lots of talk of tidying up the AngularJS code base some more, upgrading the source to use components more (now they are available), clean up the name spaces and trying to move over to more ES6 features. All of which mean we could very quickly get ourselves in a mess if we don't agree on a style for doing this and how to format the code.

Good converation started over here #4875 (comment) which you should read first. I'd suggest we document something up and get it up on the docs site. This issue can be used for a discussion on what should be in said document.

[nathanwoulfe]

Been thinking about this a bit more - maybe the easiest starting point, and quickest wins, would be to tidy up the project structure. It's pretty confusing at the moment, which slows down development and raises the barrier to entry, two things we want to avoid.

Get stuff namespaced appropriately, located appropriately, and then start chipping away at code improvements.

[PeteDuncanson]

directives.xlsx
I've created the attached spreadsheet with all the directives I could find and the modules they are saved under. I've started with some ideas (and they are only ideas feel free to rip it all up and start again) on possible module re-names.

There are a lot of directives that don't have any "umb" prefix, would love to know the history of those (I suspect they just never got a clean up).

If this is ok and helpful then I'm happy to do the same for controllers, services, filters, etc.

One issue is where to put this spreadsheet so we can all edit it safely. Pondered Google Sheets if thats ok unless anyone has other ideas. From previous efforts doing similar with projects we've done keeping a record of whats changed from then to now is super valuable (something that source control can't give you that over view as easy in one place).

[Shazwazza]

Nice! I'd be good with Google Docs, it has a history too so that's kinda built in.

So yes i think we can agree that this project is need of a cleanup. This project over the past many years, even when angularjs was in it's infancy has just organically evolved. Way back when, there wasn't really any angularjs 'experts' since it was so new, so we've all learned a thing or two since then. There was also (unfortunately) no designed/defined coding style, standard, file/class naming conventions (well... there sort of is but that's also an annoyance), module naming, folder structures, etc....

I'm all for cleaning this up, 110% and I'm sure @nielslyngsoe would agree. It's something mads and I discussed a lot too... but ... time.

I think in order to do the cleanup successfully is to make sure that @nielslyngsoe and myself along with the community who wants to help with this are fully aligned because at the end of the day the first cleanup PR is going to look very large, even it if is just moving things and renaming things. We must also consider breaking changes ( 😢 ) so it might be that not everything we want to do is possible, we can't just go renaming everything so we'll see how it goes and possible deprecate existing things and move to a legacy folder that we can't change but don't want. Moving stuff around will have an impact on both the build tools and probably the unit tests so those changes would also be part of any initial PRs.

We should strive to make any cleanup PRs as small as possible otherwise we won't be able to review them and they'll just go stale.

On google sheets you can also add true comments to cells (not just cell text) so people can have conversations there and resolve the comments when done.

[nathanwoulfe]

I think the non-umb-prefixed directives are attribute-only, while umb-prefixed are elements. I think, haven't checked to confirm.

[PeteDuncanson]

Shannon, quit apologising for this code base. We get it. Remember it works on 100,000’s of sites 😉 When I was grumbling that V8 was our “to soon” this is why. We now can’t break anything (or can we) and the major version bump would have been the perfect time to do it. I wanted to get all this stuff done as part of ingredient angulsrjs but the race was one even back then (right after Code Garden). Anyway. It is done. Will be interesting to see what we can change around. What are we worried about breaking? Packages? I’ll upload this to a google sheet and pull out the other stuff to ASAP. Then we can start commenting and get some ideas down and form a plan.

…

Sent from my iPhone On 12 Mar 2019, at 01:35, Nathan Woulfe <[email protected]<mailto:[email protected]>> wrote: I think the non-umb-prefixed directives are attribute-only, while umb-prefixed are elements. I think, haven't checked to confirm. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#4913 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABmNXtbAEIs-FxB3Ha_vtGd34nJWFZh0ks5vVwRwgaJpZM4blPry>.

[nathanwoulfe]

Seconding the no more apologies - big codebases get messy. That's life.

Should be able to tidy up without breaking stuff - if it's a like for like refactoring, we could get a lot tidied up and optimized, then save any breaking stuff for minor releases, but there really doesn't need to be anything breaking if it's approached sensibly and with enough planning...

[PeteDuncanson]

Spreadsheet can be found here:

https://docs.google.com/spreadsheets/d/1sEH0Cz59MDsMT8Fv-sDhli8I1KK79iyMFNCKhgBqVog/edit?usp=sharing

So far I've got controllers and directives in there. Not yet copied over my "suggestions" yet for the renames. Feel free to start commenting/editing.

[Shazwazza]

Cool, I'm away next week but will see if i can get some time time add comments, etc...

Re: apologising ... really i'm just trying to answer all the questions like "would love to know the history of those (I suspect they just never got a clean up).", "Its raises a lot of questions on style...", and all of the other questions of "why this?"? etc... (see #4875) . many of these answers are because of what I mention above.

Will be interesting to see what we can change around. What are we worried about breaking? Packages?

Yes, we can't just break people's back office's. Things like umb-box is used everywhere, we can't just change the name to something else. There are probably some names we can change by duplicating or wrapping to what we want and deprecating the original - but the original will still remain. We can certainly change around folder and project structures that's not an issue. We don't support packages pointing directly to view paths.

[PeteDuncanson]

While thinking about this I've been looking into setting up Prettier for the whole project (https://prettier.io/) it makes formatting delightful and consistent across js files and angularjs template files too which could be a good win. I'm going to add it to the build so it just automatically happens.

Plan would be to get it installed on a branch. Run it over ALL the javascript and angularJS templates. Check its all still working and then commit it all in as one Pull Request. This way the formatting is already done in one commit so future changes should show up just fine. Then we would ideally add it to the CI server to check if anything committed in is in Prettier format or not and fail the build if not so it would become part of the acceptance measures.

Doing the AngularJS templates too gives me a chance to do #5047 in a nicer manner as well. It all gets us a little close to having a really sweet dev setup, environment, improve readability and code quality. I'm not just adding tools for the sake :)

[PeteDuncanson]

Wow, the amount of malformed HTML in the templates that prettier is finding is eye opening! Having to correct them one at a time. One lovely gotcha is you can't use self closing tags for custom angular directives etc.

See here for some details: https://stackoverflow.com/a/25012451

Having to correct a few of those (luckily not too many). But by having this in the build chain we can stop any new ones getting through.

[PeteDuncanson]

@Shazwazza any chance you could scan through that spreadsheet and if you see anything we shouldn't be touching make a note. That way we can narrow our focus as I've had a few false starts.

In other news @base33 has started on some TypeScript goodness (base33@eafe988#diff-c62b2043de4bd80d1b2b7ca7313f38a6R99) which I'm super keen on working on as it fixes lots of goodies and he's keen to get stuck in. Why hold back when we can make this back office code base sing! :)

[Shazwazza]

Sorry pretty swamped this week, have added a reminder for this next week.

[PeteDuncanson]

I've added a new "services" sheet to that spreadsheet for the work @base33 and I are doing for #5215

@Shazwazza still swamped?

[nul800sebastiaan]

@PeteDuncanson He went on holiday, how rude (and lazy 😉).

By the way, your Google doc is restricted so nobody can read it.

[PeteDuncanson]

@nul800sebastiaan I've made it public, thanks for the spot.

[umbrabot]

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 🤖 🙂