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

V8: Get TypeScript support into the build chain so we can start porting over to it #5215

Closed PeteDuncanson closed 3 years ago

PeteDuncanson commented 5 years ago

As part of #4875 and #4913 I'd love us to start using TypeScript (https://www.typescriptlang.org/) in the back office javascript. With a code base this large and complex I think we need to start moving to a way of getting as much automated help as we can to write good stuff.

Originally raised by @base33 on the old tracker (https://issues.umbraco.org/issue/U4-11411) I think now is a great time to start getting this into Core as we can do it bit by bit. First step is getting it into the build chain and then we could just start porting over the files as we tidy them up as part of #4875 so there is no need for one mega PR with everything in TS format in one go.

Edit: The comments below contain a tonne of details and discussion. For those coming to this new here are some further reading that I've found useful that you might want to click through...

A lovely introduction for C# devs to TypeScript and why you would want to use it and the how: https://www.telerik.com/blogs/uncovering-typescript-for-c-developers

We would need to use AngularJS types so we can strongly type up all the built in goodies (scope, http, routes etc). Luckily these types already exist thanks to the awesome work here https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/angular (see the parent readme for more detail on what DefinitelyTyped is). This allows us to make angularjs built in services etc strongly typed too without having to touch their internals by simply referencing a definition file. This allows us to strongly type the scope for each directive etc. too (although ideally we will reduce the use of scope as we refactor).

When fleshing out an existing code base with TypeScript we are going to have to use the "any" keyword for types we haven't go chance to convert yet, its sort of a dirty way of flagging a type as a todo and we should treat it as such. They have their place. However if we ever got the bulk of the code base converted we should then go back and strengthen up the types by removing the "any" types for strong types. This post was a really good read on how to avoid the use of the "any" type: itnext.io/avoiding-any-in-typescript-advanced-types-and-their-usage-691b02ac345a

This one was a good read too about the 5 rules to follow when writing TypeScript, bit deep in places but worth a read, no different to learning Generics in C#: codewithstyle.info/5-commandments-for-TypeScript-programmers

Shazwazza commented 5 years ago

I'm pretty sure @nielslyngsoe would be absolutely thrilled about this

base33 commented 5 years ago

Have a PR ready on standby. Typescript compilation has been added to the build process (compiles to plain JS) and then the gulp JS step does its thing. Have also converted the notificationsService into TypeScript for others to use as an example. I will submit this later today.

base33 commented 5 years ago

Pull request submitted https://github.com/umbraco/Umbraco-CMS/pull/5225

dawoe commented 5 years ago

I'm in favor of a better structure of all the js code in the backend.

But I have a concern this will make contributing harder. Suddenly you need to know typescript to be able to make fixes in the core.

And not everybody knows typescript. Speaking for myself I never touched typescript before. I think many of the current contributors haven't either.

What does @hartvig think about this ?

Dave

PeteDuncanson commented 5 years ago

So nice to see this one so well received. Been discussing with Craig and the plan of attack is something like this:

in the reference to the right definition file at the top of the file like so: /// <reference path="notifications.service.ts" /> change its file extension to be .ts ** change any references to the original service or method to use the right types

Rinse repeat until slowly but surely we start putting types in all over the place. I don't think we will get everything but the more we can do the better and the better at it we will get. Its also a great way to explore the code base and high light any issues that we should revisit later (like why are there two different yet very similar interfaces for notifications? Surely we could merge those but we have to be careful of whats "out in the wild" so we don't break anything).

We won't refactor anything until this stuff is mostly in place so that any future refactoring we do will be easier. All in all this should be a nice bit of productive work just cut and pasting stuff around.

Any pointers gratefully received but if not we will keep cracking on. Additional hands on deck are also welcome, you don't have to be a javascript brain box to convert something over.

PeteDuncanson commented 5 years ago

@dawoe I honestly don't think that adding another tool to make a better code base should be a show stopper, why choose AngularJS in the first place if that is the logic, every choice comes with trade offs right?

The code base as is stops people contributing if you ask me hence all the issues I've been raising to try to get it all updated, straightened out and cleaned up. As you say its a bit of a head scratcher (in an understandable as it organically grew kind of way) and making fixes to it is very hit and miss and full of risk (@Shazwazza sums it up nicely here https://github.com/umbraco/Umbraco-CMS/issues/4913#issuecomment-471798427). The amount of fixes I've started and stopped because the rabbit hole simply went to deep is as long as my arm as evidence of this, you dare not change anything as you don't know the knock on effect or how to test it properly.

TypeScript will remove a massive amount of that risk by giving us a type system that will shout if we break anything when refactoring. On the whole it gets out of your way and you can very quickly learn to scan over it and ignore it. It will make it easier to do fixes and new features. In my eyes there is no down side here. Core code is just that, you won't need to use any of this in packages etc. but with a code base this big and complex it just makes sense to use whatever tools we can to make it more sensible. It grew over years and started when there were no best practises for this stuff which has a life span of another 3+ years it makes sense to get it as fit as we can. If they rebuild it today I'm 100% sure it would have TypeScript baked in from day one due to the benefits it give you. We can retro fit it and give us a longer life span on the code base why not?

If it helps it looks like C# type definitions, if you know basic C# (types and interfaces) you pretty much know TypeScript and the tool chain (added via this PR) just gets out your way, you don't even see it. It is no more of a hindrance than running the current gulp dev task.

Future PR's with backend fixes won't need to use TypeScript if they don't want to, the reviewers could add that in if needed if the PR owner doesn't have the knowledge to add it but I "got it" pretty swift and I'm happy to knock up a cheat sheet for the docs if this gets the go ahead.

I hope that is justification enough for why it should be in there and helping us rather than being seen as a hindrance. When there are only a handful of people working on the back office code base regularly is it not worth using a tool chain that helps to maximise there through put or do we need to allow for the 1000's of developers who don't want/can't learn it and who don't commit now anyway?

If you want to discus your valid concerns more I'm all ears, it important as always that the community gets thought about in everything that gets done and I applaud you in raising the query. Personally I think we should just get it in asap and let the returns speak for themselves. I'm happy to do a lightening talk at CG about TypeScript and what we are doing with the back office in general if it would help :) if not we can just discus it over a beer!

dawoe commented 5 years ago

I all for improvements. But like I said...no experience in Typescript so don't know how steap the learning curve is.

On a side note... I am just raising my concern because most of the PR i have reviewed the last year where only changes in the angular code. So that is the area where I see the community contributing the most.

On the other hand we have merged in changes that needed a patch release once it was out in the wild. Despite all the testing it would break something you would have never suspected.

If we have typescript it maybe time to start unit testing the angular part as well.

Dave

base33 commented 5 years ago

Unit testing should be quite easy once most of it has been refactored. Services, controllers and "helpers" are all classes or class-based in a sense, and so unit testing should feel very natural. It may also make people think about design before "just hacking something together" and us all regretting it later :)

PeteDuncanson commented 5 years ago

@dawoe baby steps, baby steps. I agree we should unit test but can't do it all in one hit. Incremental improvements are were we are at as time, interest and knowledge are in short supply. I'd rather we add something in that wait for the whole code base to magically get to this "perfect" state. I don't think the code base as is lends itself to being easily testable.

However, by:

And as part of all that convert to ES6, unit test, refactor and god knows what...little by little we get this to somewhere it should be. We might not get it all done but every bit we do gets its a little but tidier and so in theory a little bit more maintainable. Even if we end up rebuilding the whole back office in the future in another framework before we finish I'd hope that the work we have done will make it easier to rationalise about the back office code base.

With that, I'd suggest keep this issue focused on the task at hand (adding TypeScript to the tool chain) and take an further discussion to the forum to avoid cluttering this up anymore than needed?

jmoumbraco commented 5 years ago

All,

Thank you very much for your input on the topic of front-end code in Umbraco and TypeScript.

This is a big topic that we are very much aware of at HQ along with other architecture options for the future of Umbraco CMS, but are not quite ready to move on yet. We are looking forward to engaging in these front-end options going forward and we’ll keep you updated when we do and when additional options for collaboration arises.

I've labeled this issue with status/discussion and welcome additional thoughts and input, but be aware that we are not ready to move PRs forward at this point.

Thank you.

/Jacob

PeteDuncanson commented 5 years ago

@jmoumbraco is this issue effectively closed then or can we expect some discussion from HQ to share their thoughts? Currently we've not heard anything other than "no thanks". Difficult to know were we stand or how to progress without some more communication. My reasons for wanting to do it are pretty clearly layed out above, what parts of that do HQ disagree with that we don't know about?

lars-erik commented 5 years ago

And as part of all that convert to ES6, unit test, refactor and god knows what...little by little we get this to somewhere it should be.

I heartily approve of that part, I just don't see why TypeScript needs to be an intermediary step.

If nothing has happened since last I dabbled in Umbraco "front-end" code, there are like 1% test coverage, a horribly old, horribly slow build process and a half-baked structure rewrite to use a more "component-based" approach.

I say switch to parcel or some very light-weight build process, start getting things under test with ES6 and mocha (my latest fav), then just continue the component rewrite and delete old mud.

If anything, with a cleaner ES6 structure, generating TS typing files would be a lot easier so that package devs and end-implementors can use TS in a safe / supported way if they want.

lars-erik commented 5 years ago

That being said, I do agree that it seems like this issue is suffering from the same lack of transparency and openness as several other issues. In my experience the process of contributing to Umbraco often goes something like this:

  1. Someone in the community proposes something
  2. Someone at HQ gets personally excited and helps a bit
  3. Eager community / (personal) HQ (members) discussion goes on for a while
  4. Someone else from HQ dismisses the issue without much reason but "not focus area".
  5. Issue dies

In this case for instance, should peeps continue doing TS conversions in the PR? Have you internally decided ES6 vs. TS but won't say? Is it on a roadmap? V9? We're not after commitments, we're after something more than "No, because.".

Looking forward to discussing this over beer in a month's time. :)

lars-erik commented 5 years ago

I want to apologize for the first version of my previous post. It included an unjustified and groundless accusation of HQs motives which did nothing but paint a false picture I do not support. It did not belong in this discussion, nor any other. I am truly sorry for letting rash, poisonous thoughs like that slipping into a frindly forum. It should not have happened.

I do understand that HQ is working on a call for feedback process and that it will hopefully make stuff like this easier. What I would really wish for in cases like this is something more substantial than "we want to work on this in the future". I guess it might be too much to ask given all other concerns.

I haven't registered it in a while, but the open sprint-plan was nice. (I might juist have missed it)

A roadmap of which CFF's you think you'd start with could be a nice start given that a multi-year roadmap is probably very tough to set up. IE. will 2019 include having a look at more DI, more backoffice-GUI, variants only, or secrets features nobody is allowed to talk about until CG?

My dreams and ambitions for Umbraco, which is already and effective and agile, is to become magnitudes more effective and agile. I believe it is possible. It would be great to have some kind of forum to discuss the long term architectural plans for Umbraco, without HQ committing to anything, but just collecting good ideas and feedback. GH might be the place, but threads might grow too big. Don't have any bright ideas for that, but yeah. Keep up the good work!

Again, truly sorry for that previous reply.

jmoumbraco commented 5 years ago

@PeteDuncanson

Please consider the issue as open for discussion (but not ready for PRs) and know that we welcome and value all ideas, opinions, POCs, etc. on the topic of TypeScript in Umbraco, all very helpful input for us for future decisions to be made.

Rest assured that we are not holding anything back, so as soon as we are ready to pick this up or know more about the timing - we will let you all know. :)

@lars-erik : Love the passion - looking forward to the beer ;)

Have a great Friday.

/Jacob

PeteDuncanson commented 5 years ago

@jmoumbraco thanks for that. How best to help get something together to help inform the conversation? We want to help not hinder.

Could I suggest that as there are a number of us who would like to continue exploring the TS route we do so but in a separate fork/branch. Then we can report back on what we learn at Code Garden face to face? Assuming that would give something to discus and either way it would be fun to play with regardless. That way we can learn about any pitfalls or issues in advance.

I was pondering a video or something to show what it actually looks like in action as well for those who don't keep up to date with all the latest JS trends too. Will see how we get on.

If we like the work we can keep it and maybe merge it in, if not at least we learnt something and explored one of the options.

Have a great easter break.

jmoumbraco commented 5 years ago

@PeteDuncanson That sounds fantastic 👍 Would bring us very valuable input.

PeteDuncanson commented 5 years ago

Quick update on our findings so far:

Will keep going and keep learning. Won't have it all done before CG sadly :)

PeteDuncanson commented 5 years ago

Fork btw is here: https://github.com/base33/Umbraco-CMS/commits/typescript

Not pushed over the easter break but you'll start seeing some updates soon as both @base33 and me have been chipping away on it.

One issue we've had is where to discuss the issues we've had. Seems we can't have issues on a fork which is a shame. Anyone got any ideas?

PeteDuncanson commented 5 years ago

My fork is here btw https://github.com/PeteDuncanson/Umbraco-CMS/commits/temp-typescript and I've pushed some bits up. If you do a pull then do a npm install and then run "gulp dev" then you can see it compiles and all is good in the world. Then you can poke through and see what I'd been up to.

nielslyngsoe commented 5 years ago

Hi @PeteDuncanson

I can understand that you would be expecting some kind of response from me, though the truth is that I dont have much of a respons. I think its very usefully to bring some learnings to the table regarding rewriting the code into TypeScript. And thank you very much for pushing and looking into this. I feel very sure that we will be using TypeScript at one point, the question is just how we get there, safely.

The question wether to convert to TypeScript and how to approach it is bigger than something for me just to decide, so I think we need this type of investigation before we can guarantee a recommendation/roadmap from HQ.

Therefor I'm also very happy that you will be attending CodeGarden, and that you are proposing to meet face to face to exchange learnings. This I think will be very use-full for everyone attending that meet up.

Thanks

PeteDuncanson commented 5 years ago

Had a great night of hacking on this last night. We now have 5 services converted over and I'm getting in the swing of it. The support for TS in VSCode is a dream, full intellisense, suggested corrections, high lighting of errors and so much more. A joy to code with. Couple more issues are raised though.

Interface, these are shared "shapes" for your types in TypeScript land and are fantastic for clearly having a contract of what a method might accept.

There are a few instances of pre-fixing these with an "i" in the work done so far but the more I use the types the less I liked it. I did a bit of reading up on styles and I think we should just name them as what they are (ie Overlay rather than iOverlay) for ease of reading (https://github.com/PeteDuncanson/Umbraco-CMS/blob/temp-typescript/src/Umbraco.Web.UI.Client/src/common/services/overlay.service.ts#L93).

Trouble is where to store them so they can be used elsewhere? Currently we've just created a umbraco.services.models namespace and are stashing them in there and adding to it at the bottom of each of the services (see here https://github.com/PeteDuncanson/Umbraco-CMS/blob/temp-typescript/src/Umbraco.Web.UI.Client/src/common/services/overlay.service.ts#L14).

This allows the interfaces used and defined for each service to live in the same file as that service for easy editing but are accessible globally via that shared namespace "umbraco.services.models". This was a good quick start but I'm finding that models are quickly going to get lost and we will need some better namespacing to keep them in a logical place. I was worried about the over use of too many namespaces though as they do add a bit of noise to the code but with that noise come clarity of what type your using and where to find it I guess. So I'm tempted to play with something like this sort of pattern:

[Umbraco Namespace].[Area thing belongs to].[Thing it related to].[Interface|Class|Enum]

examples using the Overlay service:

As more and more interfaces get made though I can see this models namespace getting just as cluttered as the umbraco.services and umbraco.directives ones are. As a result I think this needs some thinking about. Again the spreadsheet (https://docs.google.com/spreadsheets/d/1sEH0Cz59MDsMT8Fv-sDhli8I1KK79iyMFNCKhgBqVog) has columns for any renames or new namespaces we create so we have a map from old to new to refer to for documentation and ease of finding. If a column is missing feel free to add it.

It will do for now though so we can keep pushing forward and finding the pain points. Thanks to TypeScript though when we come to moving or renaming them it makes it really easy to know what you've broken. Simply move the model to its new namespace and run gulp dev and it will shout at you at all the broken references something that previously just wasn't possible. TypeScript helping us out there straight away and giving me the confidence to just crack on knowing that I can safely come back and refactor later and it will have our backs. This to me is the biggest win for getting TS into the code base now. Do it without refactoring much and just pull out the types and get them referenced and then we can go back and move things around and refactor safely in a predictable manner. Getting TS in there is pretty much gentle work once you've done a few files, its pretty swift to start pattern matching and sniffing out types and then its just headphones on and hacking around, its good productive work, there are just a lot of files to touch is all. But once done then the real fun can begin :)

While we've been converting the services we've realised that we actually have a lot of options for renaming the classes or the namespace they are in to make it clearer. What you do in the file is sort of insulated as its only the name you use to register the module with AngularJS at the foot of the file that counts as far as all the other code is concerned, it just works https://github.com/PeteDuncanson/Umbraco-CMS/blob/temp-typescript/src/Umbraco.Web.UI.Client/src/common/services/overlay.service.ts#L93

Currently we are following the best practise of Pascal case for all interface, classes and enum which means renaming the service itself (ie from overlayService to OverlayService) within the file we define it as. Again we are still registering it with AngularJS with the old camelCase name for now so we don't break anything.

As an idea I tried exporting a "type alias" of the old function name for the service but it doesn't seem to work as I thought it would and given we register it with AngularJS with the old name its not needed so I'm going to strip that out in the files I've used it on https://github.com/PeteDuncanson/Umbraco-CMS/blob/temp-typescript/src/Umbraco.Web.UI.Client/src/common/services/formhelper.service.ts#L234

That's my latest brain dump from my findings so far, hope they are helping inform the conversation.

nielslyngsoe commented 5 years ago

Hi @PeteDuncanson

Just regarding interfaces, we are adding I's in front of interfaces in the C# code, so I think it would make sense to have those in the front-end code as well.

Based on my own experience with TypeScript and OOP, it also becomes handy if you have a interface and a implementation that shares the same name( Some might debate that it would be wrong if that ever happens. But I do still think that there are situations where it makes sense. Example when there only is one specific implementation, but you would still make an interface for it to use when requiring instances of that type). So you can have "IOverlay.ts" interface and "Overlay.ts" class. And in general it makes it easy to read when interface are begin refereed, I think that will be use-full in such a big project.

It would be nice to keep packages in mind. It would provide us confirmation that the choices made are right. Therefore I think we should investigate how TypeScript can work for packages, and the learnings from that would help us doing it the right way.

/my fifty cents

lars-erik commented 5 years ago

Reg. the I prefix in interfaces, my recommendation is to go with the recommended language convention. C# has it as convention. Java has not. JavaScript doesn't have them, and TS is somewhere in between the three. As far as I've seen (without researching any convention) TS convention is to not use prefixes for interfaces.

base33 commented 5 years ago

Here's a very good blog article regarding interfaces and the 'I' prefix. https://medium.com/@jonyeezs/hey-typescript-wheres-my-i-prefixed-interface-cd70efa10bd1. It comes down to behaviour and intent. In regards to the majority of Umbraco models that are passed around, most can be considered interfaces rather than concrete classes because most models are just passing properties around. Some properties are optional and some are mandatory, which is where the beauty of TS comes in because interface properties can be marked with a '?' to state whether something is optional or not on a object.

PeteDuncanson commented 5 years ago

One thing that I think is coming clear is the difference between how it "should be done" and "what can be done". As much as possible we should adopt or agree on some best practises moving forward but equally think we all know we can't just rewrite the whole code base to be perfect. My aim with all this stuff is to take one stepping stone at a time which gets us a little closer to a better code base each time, this does mean we can't make the big jumps to where "it should be" straight away. It also means we might not get as far or get there as fast or have to write more code but it will be safer code and easier to do each little step rather than a big jump to a rewrite or similar.

My idea with the changes we are doing now are we can drip feed them out in minor releases so nothing really is intended to be earth shaking, just house keeping really.

The interfaces I've been creating so far are more contracts for Date Transfer Objects (aka DTOs) than they are for "real" objects that you can create lots of. The interfaces are only used at coding time, during run time they are all removed as their work is done as the TS compiler used them at build time. This sort of works as the current code base does a lot of creating objects on the fly and passing them around. No where are these object shapes defined or validated really. They aren't documented either. So the interfaces we have created are filling in all those gaps. In an ideal world we might want to create objects that implement the interfaces but then those classes would get output in our code and then we are possibly breaking things for other users of our code. As a result I think just using interfaces to give us type safety while developing is value enough and safe enough without breaking other code.

Again, once we've interfaced up the existing code base with TypeScript then will have completed a stepping stone. Then we can think about refactoring some stuff knowing the contracts are in place to flag up if we've broken anything and where it needs correcting. We currently don't have that and so refactoring simply doesn't happen as its too "dangerous" to do :)

Currently I'm against the "i" prefix and have stripped them out but I'm not against adding them in, was just following the advice I've read.

The point about packages is interesting. As we've not changed the API as such it should all just "work", true you won't get all the TS goodness unless you loop in the definitions but its just the same JS after build so package devs shouldn't see any difference as yet? Thats my understanding anyway.

PeteDuncanson commented 5 years ago

I've added two new worksheets to the spreadsheet here for listing the new interfaces and enums we are creating. I know you might be able to get this out with a script or something but for speed and easy of seeing progress I though some old school cut and paste would do for now.

If anyone is lurking and wants to help out we do need some jsdoc style comments adding to some of the enums and interfaces. Nice easy win.

On the plus side doing all this is an amazing tour of the back office code base. Seeing so much stuff I just didn't know existing. Some of it I've no idea what it does! As usual I expect there is a reason we just need it documenting better by those that know better (or the history at least).

One thing I wish we had was some sort of dependancy graph or somethign so we could see what services depend on others. Then I could convert the leaf services first and work in. That way I wouldn't have to keep revisiting files or setting things to "any" as I worry I'll missing some of those if not careful. But for now I'm just roughing it :)

nielslyngsoe commented 5 years ago

Just to wrap up the interface talk — I had a talk with a fellow colleague and I see that continuing without Prefixed interfaces is a good option. If it happens that we will have an interface and one implementation then we will be postfixing the implementation. Like: "Overlay" for interface and "OverlayImplementation" or "OverlayImp" for the concrete class. So I'm cool going forward with that.

ronaldbarendse commented 5 years ago

@base33 @PeteDuncanson You might be able to use https://github.com/urish/typewiz to get the initial typings automatically... And probably also import typings for AngularJS, Underscore and jQuery from http://definitelytyped.org.

Regarding the other discussions: I think using TypeScript might actually make it easier to do contributions. As IDEs can give suggestions/auto completion, inline documentation, do error checking and ensure breaking something is less likely, making a change and submitting a PR can be done with more confidence, plus reviewing and testing it should be faster too.

And @nielslyngsoe, glad to see you agree on using the conventions used for TypeScript, as interfaces in TS just aren't the same as in C# (e.g. you don't have to implement them).

PeteDuncanson commented 5 years ago

Found some reading on avoiding the use of the "any" type which was a good read, dropping it here for reference.

https://itnext.io/avoiding-any-in-typescript-advanced-types-and-their-usage-691b02ac345a

This one was a good read too about the 5 rules to follow:

https://codewithstyle.info/5-commandments-for-TypeScript-programmers/

Both go into some depth and if you're new to TypeScript they might scare you to death but stick with it, no different to learning Generics in C#

rasmuseeg commented 5 years ago

Hi there, this sounds absolutely amazing!

On a side note, we should consider updating the package located at: @types/umbraco during release pipeline.

nul800sebastiaan commented 3 years ago

Thanks again all for the ideas and discussion over the years. While we're working on the new backoffice coming up in the future, our toolchain will use modern tooling and I'm confident TypeScript will make it's way in there. If you have more specific ideas for this then make sure to talk to the wonderful people on the Backoffice Community Team to give them some inspiration!

https://umbraco.com/blog/introducing-the-backoffice-community-team/