Skip to content
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

RFC: Condensing & Combining Admin Experience Modules #2556

Closed
mitchelsellers opened this issue Jan 27, 2019 · 45 comments
Closed

RFC: Condensing & Combining Admin Experience Modules #2556

mitchelsellers opened this issue Jan 27, 2019 · 45 comments

Comments

@mitchelsellers
Copy link
Contributor

Description of Problem

In the current version, DNN the Persona Bar is developed as an external entity and is installed as a plethora of components. And it is versioned separately from DNN (1.7.0 for example), however, it has strong ties to the DNN API's. It is componetized in architecture, for example, each sub-feature within the Persona Bar is installed as its own, unique, component. However in practice it isn't truly separate.

This provides a number of situations that are less than desirable.

  1. DNN Installation & Upgrade Performance : All of the additional modules have been a key contributor to the installation process growing in cost by about 50% between 8.x and 9.x
  2. A confusing admin experience for users as they appear to e "indepdnent" but really it is a package of components.
  3. Clutter in the Extensions page making it harder to find other third-party extensions for management

The tight dependency to DNN Version as well makes this appear to be a strong candidate to bring "into the fold" of the main project. And distribute as the "DNN Admin Experience." Which would better support third-party developed solutions.

Proposed Solution Option 1 : Merge PB Into a Single Extension

The first tier of proposed resolution would be to leave the Admin Experience project as-is in regards to packagaing, management, and versionoing, however, adjusting the PB to be installed as a single "DNN Admin Experience" package.

This would help cleanup the installation, and should help greatly in the improvement of installation/upgrade performance. However it would not address any of the API dependencies or developer experience issues for maintenance.

Proposed Solution Option 2 : Assume PB Under DNN Platform Repository & Deploy as Single Extension

This option would be a more aggressive endeavor, but would pull the PB insto the DNN Platform repository. The benefit here would be that incoming Pull Requests that add new features could include the API change and the UI change at the same time, and developer setup & experience should bes streamlined due to the condensed structure and less complexity.

The downside is that the build process and complexity of PB development might intimidate some that are looking to edit DNN Platform. It also sets a more formal dependency between DNN & PB.

Alternatives Researched

There are efforts underway to try and improve the developer experience, which could negate some of the need for proposed solution 2, however, the overall performance impacts at install and upgrade do not seem to have any other alternates aside from re-writing the installer.

Affected version

9.0.0 and later

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

It would be nice if just a few weeks went by without this topic coming up in yet another way. Anything that combines the PB and the Platform isn't recommended. The platform shouldn't and does not require the personabar to function.

A million things could be done to fix these issues which do not involve just coping a million new files into the main platform, adjusting all the build scripts, and a ton of other work that doesn't really address actual issues.

With a fraction of the amount of time, one could repackage the AE project to use shared packages. This would save a tremendous amount of time spend building and verifying pull requests. We could remove the dependency on JRE. We could restructure the AE solution to generate a single package and assembly. Plus a million other things which do not include just moving files.

I really don't understand why this continues to come up and waste the time those who are putting in a tremendous amount of time and effort to clean up and document the situation we're in.

@valadas
Copy link
Contributor

valadas commented Jan 27, 2019

I am all for combining the repositories, it is both a developer and management mess right now. I think we should start by merging the common components into the admin experience repository and implement yarn workspaces to deduplicate the dependencies and reduce hard drive space/download requirements. That way I think we could reference the common components from the minified bundle only in production and reference the source code in development mode. That would really help troubleshooting.

Then we would merge that into the main repository and adjust the build scripts to make sure contributors can easily build and use without having to jump through many hoops. Removing the dependency on JRE would be awesome too, that was probably put in place at a time where webpack did not exist. Also, I would empty the Website folder of the main repo and not track it, everything would build to that folder and you set that as your test site. Buidling should not product any change in tracked files.

Then for making the persona bar modules a single package, I am not sure, there might be a lot of implications. Those modules have individual permissions (even though not currently exposed in the platform UI), but that might be hard to accomplish. Maybe we could make it a single package of multiple modules, but I don't think this will change much for install time and the list of modules. Maybe ensuring all the required web APIs are in place in the platform and use that instead of having duplicated code would be nice

As for versioning, I would follow the platform version numbers just for simplicity. I do understand the idea of being able to update a single module and do a quick release of it between platform versions, but honestly the only way that could happen is if nothing has changed both in the platform and in the common components, ultimately that would be a maintenance nightmare and each module will need to declare its minimum platform version. Even worst, we did not plan the common components to be "forward compatible" in a sense that we have no way when we make a change to mark what is deprecated and is is replaced by what.

So my vote: merging the repositories and making it easier to develop, I am 100% for it.

There are also a few persons interested in putting time making at least the common components typescript (I am one of those persons), this will help developers (intellisense) and avoid many bugs (type checking).

Making it a single extension, I have mixed opinions, not totally against and not totally pro... But I prefer at this point in time to focus my efforts on accessibility, bringing back missing features, fixing bugs, etc. And improving the developer experience and simplifying debugging would greatly help everyone involved the the Persona Bar efforts.

From my perspective, merging the repositories should not be too complicated, I just have no clue how it works for npm packages, that would be something to check on the CI side of things, which I do not have any experience other than firing up a build script.

@mitchelsellers
Copy link
Contributor Author

@ohine sorry for brining this here, but there have been many offline discussions outside of the TAG group about this that I wanted to get this as part of the RFC process. Additionally I want to be clear here, there are two discussions rolled into one. One of them is as you mentioned something we have discussed. But the second part, is combining PB into a single installer, this is not something that has been discussed before.

We know your stance on any merging at the repo level, but what about merging st the installer? That, as I noted, could be done without combining repos or making any other changes

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

That’s what happened a few weeks ago with the first RFC. We came up with a plan. People donated hundreds of hours based on that plan. Now before that plan can even have a chance we’re right back to wasting more time and energy talking about a stupid waste of more time and energy.

We have talked about combining the sub-extensions into a single control panel extension. At length. Talked about cleaning up the dependencies. Lots of talk has gone on. We literally just got things to build and this is topic comes up again.

I guess I’ll stop yet again and wait for the outcome of the same talk a few weeks later. This totally sucks and will cause me to rethink investing any additional time and resources into this project if that’s how it’s being run.

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

Linking to the original issue, and yet again it's offline communications that are driving this which isn't how these items should be talked about. That's why we have RFC's. That's why we have short-term and long-term planning meetings. That's why I've donated my time into the project and before can even finish ONE project the scope gets ripped out and changed again without even the respect to reach out and talk about this offline prior to making it public yet again.

#2436

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

Take the installer, there are legitimate issues with it that we could fix. The platform shouldn't come to a grinding halt when you have 27 extensions. If that's the case, then we should modify the platform to not allow you to install more then 27 extensions. Let people know that everything will crumble with each and every extension you load into the system. Or, we should fix the installer. You know, make it actually work as intended. The location of the source code doesn't have any effect on the performance of the Platform.

@mitchelsellers
Copy link
Contributor Author

@ohine I think the point was missed here on this RFC, it’s a larger discussion of the internals.

Should PB install as 1 component or 27?

Related to this, there is another question about where it lives, I didn’t want to bring that along, but it plays into the discussion slightly as we discuss role.

NO work effort from anything done should be lost here, period. This discussion is NOT one that was extensively discussed as I’m talking about installation experience, NOT build experience. I can remove pat of the discussion on this RFC if it added confusion. But this is a NeW item that we don’t have recorded discussion on?

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

I have brought up combining the admin experience assemblies into a single installable extension, and combine the clientside dependencies into a shared workspace. These are items that we had already decided would be worked on after we got past the initial greenkeeper release. It wasn't put into an RFC because everyone agreed. Maybe it's in the meeting notes, maybe it was before we had decent meeting notes.

It also doesn't require merging the repositories to generate a single installable package for the personabar. It's doesn't play into any of the discussion unless we do not want an extensible modular framework, then for sure lets just combine everything again and undo years of work that has gone into splitting things up and making a truly extensible system which doesn't require you to use any extensions.

There is a million items that could be done which does not involve undoing the work that multiple people have put into this problem over the last 6-8 months. If that work isn't being done quick enough, just remind yourself this is a 100% volunteer effort. I've chatted with you offline on many occasions about how this isn't sustainable and how we need to work on thanking the individuals and companies who are donating an incredible amount of resources into releases. Instead of doing something with that topic, it's this one which comes up over and over again and we cannot stick to the previous decision because some magic people continue to bring it up offline.

@mitchelsellers
Copy link
Contributor Author

Again, the point is still missed. For now I’m closing this RFC and we can revisit

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

It's not a missed point. I don't understand why you continue to suggest we have to combine the AE and Platform projects. It's not required to make the changes you're suggesting.

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

And instead of making your point clear, and keeping the discussion going. RFC closed (again), and I'm sure we'll be right back into this discussion again in a few weeks. Sorry, I'm reopening it to continue the discussion.

@david-poindexter
Copy link
Contributor

Can we possibly just pipe things down a bit and be objective? I could be wrong but it doesn’t seem like anything brought up as an RFC should be personal in nature, so why is this one so sensitive and being taken as a jab? I don’t get it. It is a legitimate architectural conversation that should not only be allowed, but welcomed. It is not a waste of time and is obviously a concern for multiple reasons. If things keep escalating and aren’t productive, then people just give up and disengage. That doesn’t help the project. I recommend our posture be more resolution based than taking anything personal.

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

@nvisionative It's one that we've had a dozen times and have come to the same conclusion. We've approved work which is currently being done on my dime. So, when this topic continues to get raised without any discussion occurring with the person who is donating their time to the project. Yes, it becomes personal.

@david-poindexter
Copy link
Contributor

I don’t remember ever having a conversation about consolidation of PB modules from an installation perspective. I do remember another RFC about consolidation of the repos, but unless I am misunderstanding, I don’t think that is what is driving this. This one is from the perspective of the installation process.

@valadas
Copy link
Contributor

valadas commented Jan 27, 2019

I also do not remember past discussions about merging the installation/modules into one, just the repositories. So I think this discussion is worth having.

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

We did, I brought it up because I wanted to combine them all into a single assembly and include the react components so they're visible when the solution is open, this was shortly after I initially combined the 5 repos into 1. The argument was made that they were all standalone components and it didn't make sense to combine them. The argument was also made that the preferred method was to open up two applications. One for react and one for the services instead of working under one instance of visual studio.

Either way, all of this can be done without also combining the platform and AE repositories. They do not belong together.

@valadas
Copy link
Contributor

valadas commented Jan 27, 2019

My thought would be to move all backend code (apis and web services) into the platform, merge Dnn.React.Common and Dnn.AdminExperience into a project and move that project into the Dnn.Platform solution, everything in the same repository. That new merged AdminExperience would be FrontEnd code only (not a single c# line, no output .dll). The Website folder would be deleted and added to the .gitignore and every project would output it's result to it.

That way, you could still open the AdminExperience in another instance or just in Visual Studio Code or other IDE that is better at frontend code. But it would make it easier to manager issues and submit pull requests that affect both front and back end. The amount of .dlls would be reduced since we would not need one for each module. Using a single node_modules folder for Common and AdminExperience would save time and resources too. I think each module should be independent and have it's version bumped only when something changes for that module, that way it only upgrades it if the version is newer, which would reduce upgrade times.

One thing that is missing and may relate to this discussion is that there is currently no UI to manager which of the persona bar modules show for which users. There are PB module permsissions and menu assignments in the database but no UI to manage that. So if you want to change who can manage pages for instance, there is currently no easy way, you need to edit the database, and if you do so, then your change might get overwritten on the next upgrade.

Why do you think that the Platform and the Persona Bar do not belong together, they are tightly tied and many changes need two pull requests to make the feature available in the platform and then update the UI for it too. I think they should be together.


Take issue #2424 for instance, the problem is simple, when the user registration type is none, the welcome emails does not send to the user. This should have been a 2 line fix, 1 pull request with 2 approvers and we are done.

dnnsoftware/Dnn.AdminExperience#174 was submitted (2 line fix). The discussion went that it would be to implement this in the platform, so #2492 was created but that is only good for the backend and another PR would be needed in the front end, it got approved and merged. Then the original dnnsoftware/Dnn.AdminExperience#174 was also merged, which was a duplicate solution to the problem at hand. So a revert was created dnnsoftware/Dnn.AdminExperience#312 and approved and merged to undo that and then nobody submitted the missing part to fix this. The original person who submitted the issue still had the problem, so 2 moths after the initial PR, we need to firuge this all out and make a final fix, which was issue dnnsoftware/Dnn.AdminExperience#363 and PR #2424 . I spent half day analysing the history of this issue to submit that PR to finaly fix this problem.

Long story short, we went from what should have been a 2 line fix with 1 issue, 1 PR and 2 approvals to 3 issues, 4 prs including a revert, 8 approvals and the issue is still there, now an additional approver is needed to merge this in, so 3 persons (me and 2 approvers) need to understand the history of this in order to merge, and it took me hours to figure this one out. In my opinion having separate repositories for stuff that belongs together is a huge waste of time for management, development and testing.

@WillStrohl
Copy link
Contributor

Thank you for your objective, thoughtful, and helpful comments, @valadas. I really like and thoroughly agree with your suggestions.

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

It's a work in progress. The previous RFC #2436 was closed middle of November. We are in the middle of implementing changes which will make that situation easier to deal with on a 100% volunteer effort. If the solution we have spent the last few months working on isn't an acceptable way to solve the problem, it shouldn't of been approved to begin with. If it's not happening quick enough, jump in and help work on the solution that we already discussed and approved.

There are a million things that we could put time into which would actually benefit the entire platform besides moving files between repos in every single release of dnn.

Also, the argument for combining doesn't stop with AE. If we're going that route, we have to be consistent. We need to then combine CDF, CK, and every other extension that is the platform. The inconsistency is our biggest issue. We can't even be constant on an RFC from a few weeks ago. It's total madness and a waste of everyone's time and it's killing multiple peoples motivation to continue donating time to this project.

@valadas
Copy link
Contributor

valadas commented Jan 27, 2019

Oh, and on that previous issue, the references to the DotNetNuke.dll where old (nuget package) in the source code so the platform needs to release a new nuget package and then the AdminExperience needs to update to that pre-release version of the package, which was hard to find and not the latest. This is what took me the most time, CI takes care of bumping those version but does not commit it back in the source code, so for a local build, you need to figure this out. If it would be in the same repository, this would not be needed and all references would be current to the last commit. So to properly test a change that involves both the platform and the Admin Experience, you need to:

Make your backend changes, wait for a a merge and nuget pre-release. Then update the nuget reference to that pre-release and make your UI change.
OR
Setup local nuget packages (using a command line script that is not in the readme of the repositories) but then the CI build or the PR would still fail because of the local references.
Plus is involves 2 issues, 2 PRs, 4 Approvers, everything is duplicate.

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

Why do you think that the Platform and the Persona Bar do not belong together, they are tightly tied and many changes need two pull requests to make the feature available in the platform and then update the UI for it too. I think they should be together.

They're not tightly tied together, you can have the platform run without AE installed. With proper planning, you can have a control panel function on nearly every recent version of dnn with a single build. We could work towards a solution where we don't have 35 tightly coupled sudo extensions which are not really extensions. We have so much to clean up that just moving the repo and updating build scripts doesn't provide any real value. The real value comes from the tricky stuff nobody wants to do. Huge major changes in the installer, in creating those missing UIs, so many things we could be spending time on and we're just arguing over where the stupid source code goes without even giving a chance for the work from the previous RFC to be finished.

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

Oh, and on that previous issue, the references to the DotNetNuke.dll where old (nuget package) in the source code so the platform needs to release a new nuget package and then the AdminExperience needs to update to that pre-release version of the package, which was hard to find and not the latest. This is what took me the most time, CI takes care of bumping those version but does not commit it back in the source code, so for a local build, you need to figure this out. If it would be in the same repository, this would not be needed and all references would be current to the last commit. So to properly test a change that involves both the platform and the Admin Experience, you need to:

Make your backend changes, wait for a a merge and nuget pre-release. Then update the nuget reference to that pre-release and make your UI change.
OR
Setup local nuget packages (using a command line script that is not in the readme of the repositories) but then the CI build or the PR would still fail because of the local references.
Plus is involves 2 issues, 2 PRs, 4 Approvers, everything is duplicate.

@valadas It's all in the works to solve that, it's literally pretty much done now with @donker help and just needs some documentation and testers. It's madness that people can't just wait or ping me for status and think the only solution is just to combine everything. It really just sucks all the motivation right out of what was a fun project.

@valadas
Copy link
Contributor

valadas commented Jan 27, 2019

It's madness that people can't just wait or ping me for status and think the only solution is just to combine everything.

@ohine Don't get me wrong, I fully appreciate your efforts regarding this and you have helped me figure out complex stuff and I appreciate that. But not every contributor is in touch with you and knows about this. I also understand that is in progress and needs to be documented, it will get better then. But this only solves building the pieces that go together, other problems like managing issues on multiple repositories and being able to put a breakpoint and follow what happen is very hard right now.

There is also a lot of duplicated code. Say I am investigation an issue with the pages management, so where is my pages controller, well, somewhere here:

https://github.com/dnnsoftware/Dnn.AdminExperience/blob/development/Extensions/Content/Dnn.PersonaBar.Pages/Services/PagesController.cs

https://github.com/dnnsoftware/Dnn.AdminExperience/blob/development/Extensions/Content/Dnn.PersonaBar.Pages/Components/PagesControllerImpl.cs

https://github.com/dnnsoftware/Dnn.AdminExperience/blob/development/Library/Dnn.PersonaBar.UI/Services/TabsController.cs

https://github.com/dnnsoftware/Dnn.Platform/blob/development/DNN%20Platform/Library/Entities/Tabs/TabController.cs

Moving the backend stuff into the platform and making AE really only a frontend to use those APIs I think would be a nicer way to go and prevent this kind of nonsense.

They're not tightly tied together, you can have the platform run without AE installed

Sorry but they are tightly tied, AE is useless without the Dnn apis and Dnn is useless without some way of controlling it. That being said, I do not suggest to make AE the only way to manage Dnn, but it is the default way. Similar to how the HtmlProvider works, the provider is tightly tied to Dnn, one does not work without the other, but the current HtmlProvider is just the default one, anyone can replace it with another one if they want.

We could work towards a solution where we don't have 35 tightly coupled sudo extensions which are not really extensions

A Dnn Persona Bar Module is an official Dnn extension type. Any of them can get replaced with another one (ie EVOQ I think does it) and thirds party developers can add their own too. They are very real extensions.

@valadas
Copy link
Contributor

valadas commented Jan 27, 2019

@mitchelsellers , sorry for mixing the discussion about repositories vs installer.

How would you see the different Persona Bar Modules as one extension, given they have independant permissions and associated menu item, they each need their own destination folder and their own node in the manifest. I think building a single manifest and zip when building the project in release mode is feasable and not too complex but would that save installation time? The installer would still fire for each node right ? Or am I missing some point. My guess is this would save time for new installs a little but would not help upgrades. What would help upgrades I suppose would be to leave them separated but only bump the version on the modules that have changes, that way, only the ones that need to be upgraded would fire.

Also, Installing Dnn or upgrading Dnn for the size of sites I have is about 1 to 2 minutes, how big of a problem is this on larger distributions or on Azure small instances ?

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

They appear tied together because of poor planning when they were initially designed, we shouldn't continue that tradition going forward. Just because it's what we're using today doesn't mean it's the correct way to do it.

We've talked about a new API for probably way too many hours. That's really what is needed next. However, the planning and implementation is a huge task. One that nobody has time for at the moment to the point we stopped even asking about updates on it in our TAG meetings.

It's not difficult to set a breakpoint and follow what happens now. After your machine is configured, it's pretty easy. It's how we recommend everyone else do their development. We do not want to say, hey the only way to work with the platform is to fork the codebase, add your modules into it, because that's the easiest way to accomplish day to day development. We recommend against that for very good reason. We shouldn't be going against our own recommendations just because it's the quickest way forward.

We need to properly plan and execute just one RFC without pulling the rug out from the people working on that item, or maybe give them more then half a release cycle to complete the feature. Maybe if we can accomplish that, the momentum will continue to grow. Right now it just has me second guessing everything I've donated to the platform in the last year because people cannot stick to what we've previously agreed to as a group.

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

Installing Dnn or upgrading Dnn for the size of sites I have is about 1 to 2 minutes, how big of a problem is this on larger distributions or on Azure small instances ?

My most recent upgrade took 5 minutes from 7.1.1 to 7.4.2, it then took 9 minutes to upgrade from v7.4.2 to 9.1.1 on an Azure p1v2 instance. Azure kills the status updates at 230 seconds, after that you're left watching log files update to figure out where the process at.

This instance only has 5 users, a dozen pages, 3 portals, and NewsArticles and FNL installed. It shouldn't take that long to upgrade.

@ohine
Copy link
Contributor

ohine commented Jan 27, 2019

How would you see the different Persona Bar Modules as one extension, given they have independant permissions and associated menu item, they each need their own destination folder and their own node in the manifest.

Permissions are easy, extensions already have permission keys associated with them. We just need to use the APIs which exist in the core before the personabar was here. The decision to clone nearly all of the core functionality and duplicate it in the personabar is one of the biggest issues I have with it.

We could easily just use custom permissions which would then appear on the main AdminExperience extension, dropping that entire table and everything about it all together.

sorry for mixing the discussion about repositories vs installer.

I'm not sure why you're sorry, it was clearly suggested in the RFC and it's part of this discussion one way or another.

@sleupold
Copy link
Contributor

I haven't dived into the technical benefits of one option over the other, but for many users and developers, who want to improve the admin experience, it would be helpful to be able to replace a PB function by just replacing the install package. And being able to upgrade smaller package should be helpful as well. However, I agree that all extensions should base on common libraries.

@sleupold
Copy link
Contributor

Permission are prepared in PersonaBar, but I tested today using granular permission and it became obvious that the PersonaBar is not built using platform permission. If you have a look into the database tables, it becomes obvious, they are not based on portal properties (adminsRoleID etc) but on hardcoded role names (while we are still missing localization for it, i.e. many clients are using translated role names and have to fail on upgrades. I also dislike to have separate tables for page module and PB modules, TabModuleSettings and PersonaBarModuleSettings, ModulePermissions and PersonaBarModulePermissions.
Don't misunderstand - I would be happy to get new, improved solutions for properties and permissions, but the data structures used by PersonaBar are far from being a step forward.

@sleupold
Copy link
Contributor

IMO, we should try to separate DNN into a backend/infrastructure and a frontend layer.
The backend would include services like scheduler, search index, installer and metadata, while the frontend contains pages, desktop modules and UI components. Using this as a concept should help to be able to replace a frontend model (like WebForms) by another (Note: I am aware, the layer separation might not always be the best option, but should just point into the proper direction. Ideally, we should be able to move from one frontend model to another.

@EPTamminga
Copy link
Contributor

I personally would like the PB to remain a separate extension for the admin functionality. Merging the 2 repo's would give me the impression that the PB is the only solution for DNN Admin functionality
I hope that other admin modules will come around that are able to replace the PB and provide a better and more consistent admin experience.
We lost basic functionality due to the incomplete or ideosyncratic implementation of admin functionality in the PB. The development paradigm for the PB is different from developing 'normal' extensions, which creates an barrier for developers who want to create Admin functionality that has to go into the PB.
I also know that a lot of time went into updating and upgrading PB coding and libraries for 9.3, so any solution we opt for should take this into account.

@valadas
Copy link
Contributor

valadas commented Jan 29, 2019

I have 2 concerns about this whole idea of the persona Bar not being part of Dnn.

  1. We told developers it is an extension point and they can integrate into it, so they expect it to be there.
  2. It splits efforts if people just start making their own version of an alternate control panel instead of improving the one that is there.

I think the persona Bar architecture and container should be taken for granted and each module be its own package so that individual persona Bar modules can be replaced by alternate solutions.

I don't want to stir the pot again, I know work is being done to simplify the developer experience , but until I see it working good (I don't mean just building, but easy debugging and step through code), my opinion is that if there was a single repository, it would help a lot. We could run the source code version of react, the common components, the bundle, the module and the back end APIs in a very simple way. Nugetb packages packages would be for third party developers to use only. No more moving issues around and multiple PRs for the same issue. Also, in my opinion merging the repositories and merging the modules into one package are two separate things I think.

As for the long installations, I am curious what causes that, is there any way to profile that? Just to make sure that the amount of modules is really the cause... Is it that it causes an app recycle each time the bin folder has a change (so for each module)? Maybe we could stage all dlls of an install batch to have a single recycle?

@mitchelsellers
Copy link
Contributor Author

The long install is directly related to numbers. The quick test is to remove the PB modules and run the installer. You will see exponential improvement.

@valadas
Copy link
Contributor

valadas commented Jan 31, 2019

@mitchelsellers yes, numbers but where is the delay, is it the number of .dll files, or the number of total different files being written to disc, or the number of modules going through the installer. I am not sure what the bottleneck is, here on a local machine it takes about

There are a couple ways we can improve this depending on what the cause is:

  1. If caused by too many dlls, we could bring all backend code into a single .dll in the AdminExpercience.Library.dll, that would help there and make happy those who think the Persona Bar is not part of the platform. In my opinion it would even be better if all required webapis are available in the platform (dotnetnuke.dll)
  2. If caused by the total files, then we need to seriously think about this, currently each module has mainly a dll, a loading script and a minified bundle, merging just the dlls will only solve part of the problem, if we decide to also merge the loading script and minified bundles, then everything would need to be react and we have 4-5 modules to modify on top of merging it all.
  3. Then if the cause is the number of times the module installer fires, we can easily make one installer with all the modules in one manifest and one zip file.

I think 1 and 3 could be implemented without too much effort but I think it would be important to pinpoint the cause of the long installs to not focus efforts on the wrong cause, can't help much here locally and on my own dedicated server, it takes 26 seconds to install Dnn 9.3, I don't even see the modules install, it goes pretty much from running the sql scripts to the creating your site step.

@EPTamminga

I personally would like the PB to remain a separate extension for the admin functionality. Merging the 2 repo's would give me the impression that the PB is the only solution for DNN Admin functionality

Merging the repositories and merging the Persona Bar into the platform code are two separate things (and maybe should be 2 separate RFCs). The default html provider for instance, and all the core modules used to be in the same repo but where never hardcoded into the platform, they are extensions just like any other. Now the PersonaBar in my opinion is a bit different, we announced it was an extension point, people have developed modules for it, those developers expect it to be there on sites they have those modules. I feel that making the persona bar is optional is pulling yet another rug under developers feet. Now with this comment I mean the Persona Bar itself, not the modules it contains, those could be replaced at any time, if you build a better users module you can uninstall the default one and install yours, but the PersonaBar container and related APIs, in my opinion should be part of the platform since it is an announced extension type.

The development paradigm for the PB is different from developing 'normal' extensions, which creates an barrier for developers who want to create Admin functionality that has to go into the PB

This is mainly due to lack of documentation in my opinion. All you need is the correct info in the manifest, APIs for your backend and any front-end you like, React components are provided but not required, you could write your own plain old javascript or use React or Angular or just handlebars (some of the current modules just have handlebars). It is basically like doing any other SPA module but it is located in the persona bar instead of the page. There is a misconception (and I had it too) that you need to use React and the common components, but no, it is there if you want it, but it is optional. Speaking about barriers for developers, I think bringing Dnn.React.Common and Dnn.AdminExperience into Dnn.Platform would allow us to build and test (if we have an empty Website folder and adjust the buildscripts to do the necessary on debug builds). Imagine you clone one repository, build the users module in debug mode and your browser just opens and you are ready for live changes and breakpoints just work. Your website changes would be untracked and you can submit only one issue and one PR for a change that involves the API and the frontend. Now I know there is work being done making something similar with scripts to pull the different repositories and putting the puzzle pieces together, but I think it is a lot of work where just merging the repositories would solve that complexity and more (management of issues and PRs).

I also know that a lot of time went into updating and upgrading PB coding and libraries for 9.3, so any solution we opt for should take this into account

Yes, and my focus for the next iterations is:

  • Stop that annoying and DANGEROUS 80px push of the site a few second after the persona bar loads. I clicked the wrong button because of that way to many times!
  • Massively improve accessibility
  • Bring back anything that is missing.

@mitchelsellers
Copy link
Contributor Author

All signs point to installer execution.

Large numbers of food st runtime do not seem to have a tangible impact

@valadas
Copy link
Contributor

valadas commented Jan 31, 2019

Also, I would like to clarify some terminology to make sure everyone who comments on this RFC is aware of what is what:

PersonaBar.UI: This is the Persona Bar container and is mostly frontend code but it contains some backend code such as in installer for Persona Bar modules, menu permissions, IUpgradable code for migrating from Dnn 8 to 9 (switch control panel and create admin pages for all unkown admin pages to go into the Persona Bar). It also includes a bundle that contains React and the Dnn.React.Common components that can be used (it is optional). This bundle makes it so that we do not have to include React in the persona bar modules that use React (saves on the modules bundle size and brings a consistent look across modules) (see more detailed description below)

PersonaBar.Library: Only backend code to support general features (not module specific) such as permissions, localization, modules extension point, settings, time zone. But there is some overlap and code duplication concerning the pages, prompt module.

PersonaBar.Extensions: Contains one project per module. Each module has backend and frontend code. Each module build it's own .dll with backend code and has a frontend part. Most of them use React and produce a minified javascript file as well as a 1 div html file to contain the react app. The others use either plain javascript or handlebars. Each is bundled with it's own manifest and zipped to produce one extension for each. They are individually installed using the installer API that is in the Dnn.PersonaBar.UI.dll

Dnn.React.Common: Exclusivelly frontend code. A set of reusable React Components that module developers can use if they want a consistent look with other Dnn controls and includes some more Dnn specific components such as a permission grid, a page selector, a file uploader, etc. This is NOT Persona Bar specific, a normal React SPA module on a page could use this.

Dnn.EditBar: Allows entering edit mode and placing modules on a page. Composed of a library and a UI, it does not look to have strong ties on the Persona Bar or React or the common components, however it is displayed by the PersonaBar inside the PersonaBar iframe.

Repository: A repository is just a grouping of files, merging two repositories together does not mean they become hardcoded into each other, it is just an organizational tool.

Solution: In Visual Studio, it is a collection of Projects and may or may not have content that is outside of those projects (solution items) such as build scripts, nuget packages, node_modules, etc. It can also manage the relation between the Projects it contains (dependencies and/or build order). For instance, the Journal Module Project depends on the Library project, so if you build the journal, it will automatically ensure that the Library builds first or has no changes since last build.

Project: A collection of files that represent some part of the solution (usually that builds into a dll when talking about c# code), but it can also be a website or any of the other project types supported by Visual Studio, a project can even be of no type and just contain any file.

Dnn Extension: A zip file with a Dnn manifest and other files. An extension can contain multiple packages The manifest fires up a Dnn installer that is specific to the extension type, it can be a module or a provider or a library or just files or a combination of multiple of each of those.

Dnn Module: A type of extension that is placed in a page container and provides some feature.

Persona Bar Module: Another type of extension that provides features but that is located in the persona bar instead of a page. It is one of the official extension types. It is recommended for features that are not page specific because it has no page context by default.

Persona Bar Module Permissions: This is totally separate from any other permission types, it has it's own tables just for the persona bar. You can specify your persona bar module permissions in the module manifest, but there is currently no UI to manage them after the fact. By default (at least in the platform) modules are either visible by admins or hosts, but the architectures seems to be in place for granular permissions (found no documentation but there are tables for it).


With all this being defined, my idea is (and this could be done in small steps to not have a long release cycles):

  1. Merge the Dnn.React.Common repository into the Dnn.AdminExperience repository. This would allow us to have PersonaBar issues and PRs in one repository (avoids duplication and having to move issues around). It would make debugging the frontend part much easier because we can change the persona bar references to use the actual source code of the common components instead of a nuget package. Also, if you need to make a change to let's say the pages common component, you can view your changes directly in the module that uses it without special setup steps. Simply monitoring the module and changing the code of one of its dependencies would fire a rebuild and a refresh in the devserver. It would remove the burden of having to release a ton of versions of the nuget packages for the common components in between releases. Nuget packages would be released only on stable finished releases. The nuget package for the common components could follow the Dnn version it was released for and we would only release that when the release if finished. Since it is part of the minified bundle that ships with Dnn, third party developers would instinctively know which version to reference for which platform version they are making their module for. Right now, for every change in the common components, we need to bump the nuget version, release it, wait for Greenkeeper to pickup the change (if it does) and then approve that change for the modules to start using that new common component. That involves 2 issues, 2 PRs, 4 approvals, a nuget release; and sometimes just for a small textbox border change like in Collapsible Content in PagePicker is hiding borders when used Dnn.React.Common#217 that if you dig deep enough, required 4 issues, 8 approvals and 2 nuget releases, and slack discussions. All that for one black line, it took 10 times more time to manage it than to fix it (2 css rules). Storybooks are a great recent addition to help us, but being able to do live real testing in the context where they are used is much better. This would be incredibly helpful for the efforts I am going to put in making the common components accessible.
  2. On that newly merged Dnn.AdminExperience repository, consolidate the node_modules folder using yarn workspaces. This would basically have a single node_modules folder for all javascript dependencies and deduplicate them. If you need let's say react-moment.js in 15 modules, you only download it once, you need react-dom or redux, etc, it deduplicates them by about 20 times. This would speed up builds and reduce hard drive usage by a lot! Each module would keep it's own dependencies, this does not change anything other than doing deduplication. If you need different packages or different versions of the same package on individual modules, that still works.
  3. Separate backend code from fontend code in the Persona Bar modules. This would generate a single dll for all of the modules, it would avoid code duplication, one example that comes to mind is the pages WebApi and controllers, this is duplicated at so many places, adding a feature to the pages would involve changing backend code at all those places. I am sure it would also help with performance, right now accessing page data involves a call from the module webapi to a module controller that calls the admin.library api that calls the core api and the same way back out. My vote it to have WebApis in the core, that would help with permissions/security and would make them accessible to SPA modules too (given required permissions, they don't have to be Persona Bar specific). But if the decision is made to really keep that separate, then at least consolidating those webapis in the Dnn.AdminExperience.Library.dll would help performance, deduplication and cleanup.
  4. On the Dnn.Platform repository, move the content of the root Website folder into the Platform/Website folder so that we can delete and ignore the root Website folder.
  5. Ensure each project build scripts copies changed files (it is easy to copy only changed files) in that root Website folder, at the location it should go on an uninstalled site if built in release mode, or at the location it should go on an installed site if built in debug mode.
  6. Merge the consolidated AdminExperience repository into Dnn.Platform reposisoty in an AdminExperience folder. Adjust build scripts (actually, I think they are already like that) to output to that root Websites folder
  7. Since all of the PersonaBar modules would now be only fontend, all the files would be in Website\DesktopModules\Admin\Dnn.PersonaBar\Modules making the release package would be just zipping this whole folder in a Resources.zip and creating one manifest that unzips those files to that folder and register each PersonaBarModule (One multi-module extension but with a single zip and no dlls).
  8. For each Persona Bar module, adjust the yarn start command to edit the script that load the bundle in order to load the devserver version instead of the minified version, that would be in the Website\DesktopModules\Admin\Dnn.PersonaBar\Modules\ModuleName\scripts\modulename.js Revert that change when running yarn build.

Contributing documentation would change for 5 long videos (about 3 hours of watching, and already outdated due to changes) to those simple steps:

  1. Install a git tool for your platform, run git clone https://github.com/dnnsoftware/Dnn.Platform
  2. Open the solution in visual studio and build in release mode.
  3. Create a new website in IIS using a domain that resolves locally (such as localhost or something.localtest.me), set the folder of the site to be the root Website folder of where you cloned Dnn sources.
  4. Adjust the Website folder permissions to allow full rights to the worker process (usually "IIS AppPool\TheNameYouGaveYourSite".
  5. Navigate to your website and run the installation wizard.
  6. Do your change and build just that project in debug mode, to step through code, insert a breakpoint and attach visual studio to the worker process. Refresh the site and test.
  7. For the PersonaBar modules, navigate to the folder in the terminal or command prompt (or open it in your prefered ide such as Visual Studio Code, run the "yarn install" command, then "yarn start", refresh the site and navigate to the module. Changes are live, no need to build until you kill the yarn build command. When done testing run "yarn build" so your site continues working after killing the "yarn start" process.
  8. To reset the site to a new install state, delete the contents of the Website folder, build the solution in release mode and navigate to the site again.
  9. Your prefer to have your test site elsewhere? Delete the root Website folder and make a symbolic link to the location of your choice.

@valadas
Copy link
Contributor

valadas commented Jan 31, 2019

@mitchelsellers can you define food st runtime :) I guess that was some autocorrect...

@EPTamminga
Copy link
Contributor

EPTamminga commented Jan 31, 2019

@valadas

THANK YOU for the extensive explanation about and documentation of the PB!

@valadas
Copy link
Contributor

valadas commented Jan 31, 2019

You are welcome, I just want to make sure that people who have opinions about this subject understand the current architecture and mean what they mean. There are a lot of parts to this puzzle.

One other thought came to mind and correct me if I am wrong, it would not be the first time and this is just a theory. Could it be that the performance issues with installing the persona Bar modules is caused by the fact that there is not known built-in installer. The installer has to use reflexion (so load all the assemblies) to find if there is an installer that matches that package type, and it does it for each module and since each add a new dll, the next as to again reload all assemblies to redo the reflexion thing again? I say that because other extension types do not have to use reflexion to find the installer...

@bdukes
Copy link
Contributor

bdukes commented Jan 31, 2019

Thanks for your detailed and thought out info and proposal, @valadas. I don't have a strong opinion on most of these changes, but I do have a few responses to the proposed plan:

  1. For Dnn.React.Commom, I assume you mean npm packages, rather than NuGet packages, unless I'm missing a piece in the puzzle. The recommended way to develop a package is using yarn link, which creates a symlink in the background and, once you know to do it, seamlessly uses your local version of a package. Since we still want to publish Dnn.React.Common on npm, I'm not totally sure how much would change if it was moved into the AdminExperience repo.
  2. Adopting yarn workspaces seems like a no-brainer, regardless of how any projects end up being structured.
  3. Combining the C# projects to produce one DLL for all of the projects sounds like a small change that could help the development experience a lot (with a minor tradeoff of reduced flexibility). 👍
  4. Does this require any major, time-changing build script changes?
  5. This would be done in MSBuild? Are there client-side files which MSBuild doesn't know about that would be affected?
  6. Regarding the Persona Bar being a required component or not, I don't think there's any push to remove the Persona Bar as an option for developers to create extensions against, so I don't feel like the rug is being pulled out from under anyone. However, folks who want to use alternative admin experiences should be able to do so. Regarding the organization of the code in the repositories, I don't have strong opinions, but I do think we need to make sure that we're working with @ohine and @donker so we're not throwing away work they've invested in improving the development experience.
  7. 👍 sounds good
  8. I have some concern about automatically making changes we don't want to get checked in. In my experience, folks (even with lots of experience) will forget and just commit all changes, and then things mysteriously don't work all the way for no clear reason. If there's any way to automatically switch things based on config, so it doesn't change any source files, that would be strongly preferred.

@ohine
Copy link
Contributor

ohine commented Jan 31, 2019

  1. This is already handled in the configuration script. Peter wired up yarn link.
  2. This is already planned work from multiple TAG meetings over the last few weeks.
  3. We have a huge performance issue with files and folders. That's why we have to use a non-default fcnMode setting to keep the application from constantly recycling the application pool. Any work done to reduce our installed footprint would help performance tremendously.
  4. Yes, that's the folder that's used to generate the install and upgrade packages.
  5. Some projects do this, some don't. It's part of the constantly we need to apply across all projects. Not just AE/React/Platform. Everyone forgets about CK and CDF when debating this topic.
  6. Sorry, but absolutely not. Without a huge rewrite to the AE project, I will continue to recommend against this. There are more intelligent ways as previously discussed to solve the issues.
  7. Adjusting the build scripts is easy, but there are still more intelligent ways to solve these problems. The project itself needs quite a bit of work before we should talk about changing the build scripts. After the AE teams delivers any of the recommended and approved items for improving performance, I'm happy to work on adjusting the build scripts to make sure we can include those changes in the release they're merged into.

Quite a lot of these points are also already solved on the Dnn.DevelopmentConfig that we've discussed on the TAG calls and privately. If anyone wants to review it, kick the tires, provide feedback, etc. You can find it all on the repo. The plan was after it's ready, we'll move it into the main dnnsoftware repo and update the main documentation to include the finalized steps.

@valadas
Copy link
Contributor

valadas commented Jan 31, 2019

@bdukes

  1. Sorry, yes, I meant npm packages. Yarn link was my first solution, but at some point it stopped working for me, not sure why. I would need to delete everything and retry I guess. We still want to publish to npm but we would not need to do it 50 times between two releases and we could have the package version match the dnn version it was released for. Third party developers currently have no way to know which version they need to reference for which dnn version. Sure we can document that somewhere, but it is one more step.
  2. Yes
  3. Yes
  4. Well, we just need one task that copies the content of the Platform\Website into the Website folder on release build.
  5. I am comfortable doing it with msbuild but that should also work with cake if that is the way it ends up going.
  6. Yes, the proposed changes would change nothing, hence my previous comment that merging repositories does not mean merging the code. However, there seems to be interest to move the administration WebApi services into the core, if that is the case, those are sometimes tighly planned for a specific UI. It would still not lock in the persona bar into the platform and one provide 1 set of apis for administration, if people want to replace the persona bar, they can and will have those apis available if they match their needs, if not they can write their own. But yes, those who would choose to remove the persona bar would not be able to use any module that was made for the persona bar. So if I build and sell an awesome modern module that goes in the persona bar but my clients do not have the persona bar because they liked something else, I am out of luck and need to build my module for all the available control panels out there. Anyway, I am strongly agains this idea of alternat control panels, but the proposed changes here would not prevent that from happening if that is the direction the community wants to go. I just expect it to be there on sites as a developer because it was announced as a dnn extension point.
  7. That change would be on the root Website folder that is untracked and would not cause a tracked change, just you test site would load the deveserver version automatically if you ran yarn start and the minified version if you ran yarn build.

@valadas
Copy link
Contributor

valadas commented Jan 31, 2019

@ohine

  1. Yes and it was working for me too but at some point it stopped showing the source lines in errors for the common components when used in modules. I am not sure why, it may be a change in the webpack configs or something local here. I guess I have to delete and retry that.
  2. Yes and we do seem to have consensus on that, just wanted it part of the steps of the proposed solution.
  3. I am not sure what you mean and I am not familiar with fcnMode, By reducing the footprint do you mean file sizes or file amounts. Are you leaning towards combining the APIs in the core, growing dotnetnuke.dll or the opposite to have smaller dlls? Or something in between, combining them in dnn.adminexperience.library.dll ?
  4. Cool
  5. Correct
    6 and 7. Ok, we have no consensus on this idea and there is work done currently for an alternate solution that involves https://github.com/nbrane/Dnn.DevelopmentConfig/wiki/Setup-Your-Workstation and issue.sh to simplify management.

So let me make action items on at least the items we do have consensus so we can start moving on those.

@valadas
Copy link
Contributor

valadas commented Feb 1, 2019

So here are a couple things we seem to have consensus on and that I am moving into issues:

Still need to get a consensus:

Move backend code out of each PB module
There seems to be an agreement that having one dll per module is bad and we need to combine that code somehow

  • One option is to move it all into one of the existing PB libraries, either Dnn.PersonaBar.Library.dll or Dnn.Personabar.UI.dll
  • Another option is to move it all into the core, but that blurs the lines if we decide that the persona bar is an optional extension.
  • A third option is take it into a 2 step process. First move it all into a new combined Dnn.PersonaBar.Modules.dll and save on having to have one dll per module quickly. As a second step we could focus on cleaning/deduplicating/moving code where it belongs. Maybe keeping WebApis outside the core but make them all connect directly to a core api, and improve that api if needed.
    This needs to be done if we decide to make the persona bar modules a single package so we register a single assembly instead of 20 during installation.

Merging AdminExperience into Dnn.Platform
There is no consensus and alternatives are being worked on.

Making the PersonaBar modules a single package
There seems to be consensus but we need to resolve the moving of the backend code before that happens.

Automate switching from devserver to minified
This may be fixed by the https://github.com/nbrane/Dnn.DevelopmentConfig project so it is being worked on now.

@mitchelsellers
Copy link
Contributor Author

Based on the decisions outlined above, and the associated issues that were created by @valadas I am going to close this RFC.

More work is needed for the Persona Bar, however, it has been discussed that any major structural changes, outside of the ones with issues already created, should be held until the environment can stabilize and we can validate the effectiveness of other changes in development environment improvements and other changed.

Once the already agreed upon changes have been made, we can revisit any other changes if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants