search

umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.47k stars 2.69k forks source link

Start decoupling from AngularJs #7718

Closed nathanwoulfe closed 3 years ago

nathanwoulfe commented 4 years ago

Might be controversial, but here we go...

I'm proposing we start refining the backoffice javascript to remove/reduce the coupling to AngularJs. The quickest win here (IMO) is to remove all usages of Angular functions.

Most (if not all) of these exist as native JS methods in ES6, or are provided in Lodash. Umbraco currently takes a dependency on Underscore, of which Lodash is a superset. Benefit of Lodash is that it allows individual functions to be included, greatly reducing the size of the dependency. I don't see any issue in replacing a framework dependency with a library dependency.

So I guess this is two-fold - replace AngularJs fns with vanilla JS where possible, else import the appropriate Lodash method. At the same time, replace Underscore with vanilla, falling back to Lodash.

Why? It will need to happen eventually. Regardless of what direction the backoffice takes, the AngularJs functions will stop working. These changes can be addressed now as incremental improvements rather than leaving until later as part of sweeping backoffice changes.

TODO:

anthonydotnet commented 4 years ago

Agree.

These are quick wins.

c9mb commented 4 years ago

I'll be even more controversial... migrate angularjs to vuejs - for which the above is a compatible step of an increment migration process. :scream:

nathanwoulfe commented 4 years ago

@c9mbundy even mentioning an alternate framework is waaaay out of scope for what I want to do here - these changes would be part of the required groundwork for shifting the backoffice to Vue, React, AngularX, JsDuJour * etc.

Really want this to be a completely stack agnostic conversation, focus on cleaning up/improving where possible to best facilitate whatever migration happens in future

* this does not exist. Yet. Consider this me staking my claim to the name!

pgregorynz commented 4 years ago

Agree with this.

The less framework dependency the better.

In ideal world it would be really nice if plugins were completely agnostic of framework used but that there would be a simple interface for passing the model. Not sure how possible it is but dreams are free, although I know @PeteDuncanson did something like this a while back to run React in the back office?

giuunit commented 4 years ago

Agree with this. Slowly removing framework dependencies is the only way forward

dawoe commented 4 years ago

Hi @nathanwoulfe

Same can be done for the usage of underscore.js. A lot of the functions in there are native as well.

Dave

nathanwoulfe commented 4 years ago

@dawoe I know there's no native deep copy (lodash provides that) but outside of that, I think most everything is available native

dawoe commented 4 years ago

As I understand the clone function from underscore is not a deep clone : https://underscorejs.org/#clone

I always use angular.copy for that

Dave

nielslyngsoe commented 4 years ago

Hi @nathanwoulfe This is great, and it's a great step towards moving forward.

Here are a few perspectives of why we think its right:

I think you wrote a perfect description of the first goal for this journey:

Replace AngularJs functions with vanilla JS where possible, else import the appropriate Lodash method. At the same time, replace Underscore with vanilla, falling back to Lodash.

Feel free to get started, I would love to see a few minor PRs to ensure that we agree on the path. And afterward this could go for full-speed with up-for-grabs issues etc.

anthonydotnet commented 4 years ago

This is a great initiative.

Now all we need is the blessing from HQ, and a few hackathons.

Then in a year we can discuss what ever hipster JS framework is around.

nul800sebastiaan commented 4 years ago

And just to set some expectations; we don't have anyone dedicated to "new hipster JS framework" at the moment. The work resulting from this will be processed by the ~PR Team~ Core Collaborator team and it's easiest if this is happening in small PRs. The priority for evaluating incoming PRs is on fixing current bugs first.

And while I do love some good cleanup work, it's dangerous, due to lack of automated testing, so unit tests will help a lot in making getting changes evaluated and merged more quickly.

nul800sebastiaan commented 4 years ago

Last note: while this is up-for-grabs, this issue will not be "fixed" by a single PR. It would be good if @nathanwoulfe kicks of with a few trial PRs and then to come up with a list of items that need improvement, so that other people can pick up various smaller bits to improve.

Please organize this so that the work is easy to share among a few people. For inspiration, the accessibility team is really good at making a list of stuff like that: https://github.com/umbraco/Umbraco-CMS/issues/5277

umbrabot commented 4 years ago

Hi @nathanwoulfe,

We're writing to let you know that we've added the Up For Grabs label to your issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post

Thanks muchly, from your friendly PR team bot :-)

markadrake commented 4 years ago

Just some thoughts as I read through this.

PeteDuncanson commented 4 years ago

Wow. I'm excited to see this one get raised and get such a good reception. Bravo Nathan.

Small units of work would be the order of the day if we wanted to get this in pulled in. In my mind it would be super tiny changes per file (maybe even per method if its a big file to change) so that the CC team can easily eye ball it and pull it in. Plus if the changes are small enough you can get a few done in a lunch break and feel like you've achieved something and its gets HQ's PR count up so they can shout about that, win/win ;)

Testing is always lovely but its also difficult to create tests for some of the existing stuff the way its written (which is one of the reasons its not been done as yet). I think we would need someone brave to do a bit of a sprint into how best to do the testing and then show an example that others can follow. I don't think it should be a requirement though (swapping out one forEach implementation for another shouldn't require a test in my mind), but encouraged if possible.

For completeness for those that are hearing about work on tweaking the back office here are some of the older issues that contain lots of thoughts/ideas/plans on whatever we can do to polish up whats already there and get it in a fit state:

https://github.com/umbraco/Umbraco-CMS/issues/4913 https://github.com/umbraco/Umbraco-CMS/issues/4875 https://github.com/umbraco/Umbraco-CMS/issues/4875 https://github.com/umbraco/Umbraco-CMS/issues/4804 https://github.com/umbraco/Umbraco-CMS/issues/4872

and the dreaded "Get TypeScript into the back office" thread https://github.com/umbraco/Umbraco-CMS/issues/5215

cssquirrel commented 4 years ago

Agree with all the agreeing. ;)

As has been already said, regardless of the backoffice's future form, it will definitely part ways from AngularJS at some point. This feels like a real approach to making an actual start to the process. Which is fantastic.

protherj commented 4 years ago

h5yr @nathanwoulfe, sounds like an excellent step to take.

nathanwoulfe commented 4 years ago

Given this was a whole lot less controversial than I might have thought, I'm happy to take a lead on kicking off the work. Will follow the A11Y team's lead and go for a Trello board to manage work.

First though, will do some proper analysis of how much work is involved, where the quick-wins are hiding, and changes that may still require Lodash. From that, can map out a plan in Trello and away we go. Easy. Simple. 😃

More than happy to talk further about how to approach the work - by file, by function, by module etc.

nathanwoulfe commented 4 years ago

Trello board for anyone interested. Feel free to add/change/move stuff. I'm not a PM.

ronaldbarendse commented 4 years ago

While I absolutely agree we should get ready for a client-side framework transition (especially because the AngularJS LTS period ends on June 30, 2021), I'm a little sceptical whether any of these code changes will eventually be part of the new codebase and/or add any real value.

Sure, it doesn't hurt to rewrite some JS functions and having additional tests would be great, but I think that time would be better spend in converting existing code to TypeScript (as @PeteDuncanson linked in his comment). That has the potential to reduce/prevent a lot of bugs, make testing easier and while doing so, also swap out a lot of these functions at the same time!

nathanwoulfe commented 4 years ago

@ronaldbarendse as much as I'm on board with a TS migration, that's an order of magnitude more work than getting these changes done. Regardless of where the backoffice goes in terms of framework/language, these changes will need to be made, so no time like the present.

Sure, some/all of this may be removed in the future, but until that happens, it's still an incremental improvement to reduce the coupling to AngularJs.

anthonydotnet commented 4 years ago

Agree. In order to TypeScript the code, we need the code to actually be written in JS. Currently it's full of legacy AngularJS functions. You can't TypeScript those.

Moving to as much vanilla JS as possible is the first step to actually using TypeScript.

nielslyngsoe commented 4 years ago

I think everyone is in for a TypeScript codebase. But I would say that we first need to make sure that we are in a good state for implementing TypeScript.

I would love to see us making an effort to decouple things as the main focus. Just like Nathan now proposed getting rid of utility functions from AngularJS. The next step could be moving Services out of AngularJS and splitting some services up to minor, or just turning some functionality into functions. And in this process, it could make sense to convert to TypeScript as well. But I think we are better of not focusing on TypeScript as the goal but focusing on decoupling. Ensuring that our architecture becomes SOLID, or at least closer to those concepts.

MMasey commented 4 years ago

PR for isString #7929

MMasey commented 4 years ago

PR for isArray #7934

liamlaverty commented 4 years ago

PR for equals and some other utilities used by equals can be found here #7975

bjarnef commented 4 years ago

PR for fromJson https://github.com/umbraco/Umbraco-CMS/pull/8014 has been merged to v8/dev but is missing to be included in v8/contrib.

The replacement of angular.fromJson with JSON.parse in https://github.com/umbraco/Umbraco-CMS/pull/7952 was breaking a few things in core since angular.fromJson also checked is value was a string before doing JSON.parse.

Since the Utilities already had the toJson I think it should also have this fromJson function.

kjac commented 4 years ago

Bit more work on replacing angular.forEach in #8501

kjac commented 4 years ago

And more angular.forEach replacement in #8505

nathanwoulfe commented 4 years ago

@kjac lovely stuff! I've picked up some similar changes from an issue @warrenbuckley created a while back re removing underscore, so have been chipping away at killing off _.each. Long story short hopefully we don't step on each other's toes.

I'll link the PRs here when I'm not on my phone. Thinking maybe a parent issue to link the angular and underscore stuff might be useful too

kjac commented 4 years ago

@nathanwoulfe I saw the PRs - #8475 and #8490 ... I think we're good 👍 the _.each-removal doesn't seem to interfere much if at all with the angular.forEach removal. And if it does, well... it should be merge-able for sure.

nathanwoulfe commented 4 years ago

@kjac sounds good to me 👍👍

kjac commented 4 years ago

8538 contains angular.forEach replacement for the remaining property editors.

kjac commented 4 years ago

8552 contains angular.forEach replacement for filters and services

emma-hq commented 3 years ago

Aw this issue!!! I love it. Brings back some nice memories from last year and we all know they're in short supply. We have only one more to do and then PROFIT! 😆

Anyone from this team want to pick up .extend()? Go on. Go on go on go on go on go on.

Em

bjarnef commented 3 years ago

@emma-hq I couldn't find an open PR to replace angular.extend() so I have submitted a PR here: https://github.com/umbraco/Umbraco-CMS/pull/10023 😁 👍

emma-hq commented 3 years ago

Aw thank you @bjarnef - you're a ⭐

nathanwoulfe commented 3 years ago

CLOSED! Finally!

Thanks to all who contributed PRs/thoughts/prayers 😄

nathanwoulfe commented 3 years ago

10759 resolves some sneaky angular.forEach usages which managed to weasel their way back in