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

initiate the process of applying angular style guide #1168

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

yeiniel
Copy link
Contributor

@yeiniel yeiniel commented Dec 10, 2020

this is the second step on the roadmap to upgrade to the new version of the angular framework. It contributes to issue #1078

during the migration to the new version of the angular framework we will need to load both applications (angularjs app and Angular app),connect them and migrate each component, factory and service one by one. This PR contributes to that objective by splitting each application element into its separate module (as directed by the angular style guide) and making explicit the dependencies between those modules. This will help in the future by allowing the migration of each individual factory, service and component without having to touch other code. Additionally this simplify the conversion of the code to typescript by making the process incremental and more granular.

At this time the entire job of separating all the elements is not complete. My idea is to merge this PR in this state to allow myself to start working on the typescript migration and at the same time other members of the community can contribute with finishing the job of separating each element into its own file. It's a relatively simple process except for small chunks of code that require some though.

Let me know if we can proceed this way or if you want me to complete the splitting of the code.

I know this explosion of files could look ugly to you considering how the application was structured but once the build process has been set in place is not going to impact application performance. Additionally it helps identifying not used code, circular dependencies of modules (not allowed on the new Angular framework), etc.

@yeiniel
Copy link
Contributor Author

yeiniel commented Dec 10, 2020

please take a look at the changes to the bufferResume service. it belonged to the bufferResume module (a dependency of the weechat module). That service was moved to the weechat module. The reason is that bufferResume service depends on the settings factory that is provided by the weechat module and that constitutes a circular dependency.

@cormier
Copy link
Member

cormier commented Dec 24, 2020

Hi @yeiniel. Sorry for taking so long to get back to you. Thanks for the great work ! I will review the PR and give you specific comments, but the approach you describe looks good to me.

Let me know if we can proceed this way or if you want me to complete the splitting of the code. My idea is to merge this PR in this state to allow myself to start working on the typescript migration and at the same time other members of the community can contribute with finishing the job of separating each element into its own file. It's a relatively simple process except for small chunks of code that require some though.

Sure. I believe we can proceed this way, but that before merging this PR we need to spec the work that still needs to be done so that everyone can easily know what has been migrated and what still needs to be migrated. I will review the PR and then let you know more precisely what I mean.

Also I have noticed that you have merged origin/master in your own branch. Would it be possible to rebase your branch on top of origin/master instead, as is it the convention we used in the project ?

Cheers

@yeiniel
Copy link
Contributor Author

yeiniel commented Jan 9, 2021

Hey everyone. Sorry for the delay i replaced the merge by a rebase. The work to be done is as follows:

  • separate conditionalLinkify filter into it's own file
  • separate DOMfilter filter into it's own file
  • separate getBufferQuickKeys filter into it's own file
  • separate WeechatCtrl controller into it's own file
  • separate IrcUtils service into it's own file (consider moving it into the weechat angular module)
  • separate $store factory into it's own file (consider moving it into the weechat angular module or even better replace it by one of the open source modules available on npm)
  • rename models file (append type) (consider moving it into the weechat angular module)
  • rename notifications file (append type)
  • rename plugin-directive file (replace - by .)
  • rename settings file (append type)
  • rename utils file (append type)
  • rename websockets file (append type)
  • rename whenscrolled-directive file (replace - by .)
  • move filter, service, factory declarations into the main angular module file. this makes clear the dependencies between artifacts. take a look at the already separated artifacts to get ideas of how to keep dependencies declaration with the function definition (handlers factory is a good example)

After all of this is done focus need to be on converting directives into components. That's a more difficult process

@torhve
Copy link
Member

torhve commented Feb 19, 2023

Hey, @yeiniel , would you be willing to rebase this on top of current codebase? I would be interested in reviewing and helping out, with the goal of eventually modernize this code base

dependabot bot and others added 13 commits May 9, 2024 19:11
Bumps [express](https://github.com/expressjs/express) from 4.18.2 to 4.19.2.
- [Release notes](https://github.com/expressjs/express/releases)
- [Changelog](https://github.com/expressjs/express/blob/master/History.md)
- [Commits](expressjs/express@4.18.2...4.19.2)

---
updated-dependencies:
- dependency-name: express
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
this works toward complying with rule glowing-bear#1 define one component per file

additionally it fix the dependency graph between files making obvious the dependency between the angular module and its constituends
this change fix a circular dependency bettwen the bufferResume  module (using the settings service from weechat module) and the weechat module depending on the bufferResume module

aadditionally the buferResume file has been renamed to show the nature of its content
@yeiniel
Copy link
Contributor Author

yeiniel commented May 10, 2024

@torhve sorry for the delay. done

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

Successfully merging this pull request may close these issues.

None yet

4 participants