Refactoring core.ts

[BassemHalim]

Hi,

While working on another PR I noticed that core.ts was extremely long and it was a little hard to figure out how to implement some changes so I am planning on refactoring that code into multiple modules to make it more maintainable.
For example, core.ts has 98 functions

$ grep -c "= () =>" src/livecodes/core.ts
98

So I am thinking we could extract some modules from core.ts like a ConfigManager, EditorManager, StorageManger, etc. without changing the functions and then later we can refactor the long functions.

This might be too much for a single PR so maybe break the PRs into something like extracting ConfigManger from core.ts, extracting x etc.

What do you think?

[BassemHalim]

Also, I haven't checked all the tests but we might need more tests to ensure that nothing breaks in the process

[hatemhosny]

Thank you, @BassemHalim
I agree, core.ts file has become really big.

I am open for suggestions and let's discuss the details here before PRs.

[BassemHalim]

To start, we could benefit from a ConfigurationManager class that would have the following methods

class ConfigurationManager:

• getConfig()*
• setConfig()
• updateConfig()
• loadConfig()
• applyConfig** // maybe

class User (could be a separate class or part of ConfigurationManager

• setConfig ()
• loadConfig ()

* I noticed getConfig gets called a lot and it creates an object clone everytime which is not always necessary so we could have a readonly config and a getCopy() for the clone

[hatemhosny]

Hi @BassemHalim

I do like grouping related code. However, I'm not a fan of classes in JS.
We can just keep them as functions and they can be grouped as ES modules or by the directory they are imported from.
If they need to share some internal state, then I prefer the revealing module pattern: see examples here and here
This is a personal preference and to keep consistent with the rest of the codebase.

I discuss the rationale in this video: https://youtu.be/En9IobtyMd0?si=RiAHJsriRcCKmaH9
This is a part of this playlist: https://www.youtube.com/playlist?list=PLj_e60Rh_muA93LtAeRL3BZgsnQtP42Jl (self-promotion!)

So we can group related functions in the same directory.
Many of the functions in core.ts mutate variables that keep the state.
This can be handled by passing callback functions that can do that. This way they should also be easy to test.
We just need to be wise enough so that our code doesn't get bloated by irrelevant callbacks.

Handling config calls lots of other functions and operate on many variables and is important for the core functionality. I would keep these in the core.ts file for now.
Maybe we can start by moving most of the handlers. They probably operate on less shared code.

Please let me know what you think.

[BassemHalim]

Interesting take! I agree js is weird in many ways. Ok sounds good, I don't have a strong opinion towards any of the paradigm so I can use modules.
I will start looking into moving handlers and will see how it goes.

[BassemHalim]

Hi Dr @hatemhosny

While trying to extract the handlers from core.ts I noticed that many of them still depend on state in core.ts such as EventsManger which makes me have to pass many variables to the extracted handlers and I feel like it will make it less readable so I was thinking of updating/creating some modules that mimic the singleton pattern but using modules

for example, for the EventsManager I would move events to be a module variable that way we can create multiple EventsMangers and they would all be sharing the same events array and that would help with the refactoring because I wouldn't need to send lots of parameters to the extracted basicHandlers. I would do that to other state variables like editors if necessary. Alternatively, global variables should be moved to some kind of main function and passed down to the functions. What do you think?

import type { EventsManager } from '../models';

 const events: Array<{
    element: HTMLElement | Document | Window | FileReader;
    eventType: string;
    fn: (event: Event) => void;
  }> = [];


export const createEventsManager = (): EventsManager => {

  const addEventListener = <T extends Event>(
    element: HTMLElement | Document | Window | FileReader | null,
    eventType: string,
    fn: (event: T) => any,
    options?: any,
  ) => {
    if (!element) return;
    element.addEventListener(eventType, fn as any, options || false);
    events.push({
      element,
      eventType,
      fn: fn as any,
    });
  };

  const removeEventListener = <T extends Event>(
    element: HTMLElement | Document | Window | FileReader | null,
    eventType: string,
    fn: (event: T) => void,
  ) => {
    if (!element) return;
    element.removeEventListener(eventType, fn as any);
    const event = events.find(
      (ev) => ev.element === element && ev.eventType === eventType && ev.fn === fn,
    );
    if (!event) return;
    events.splice(events.indexOf(event));
  };

  const removeEventListeners = () => {
    events.forEach((event) => {
      event.element.removeEventListener(event.eventType, event.fn);
      events.splice(events.indexOf(event));
    });
  };

  return {
    addEventListener,
    removeEventListener,
    removeEventListeners,
  };
};

[hatemhosny]

Thank you, @BassemHalim
Sorry for the delay. I was busy the past few days.

I see your point. While this does work, I think it will only solve a part of the problem.
What about other dependencies? e.g. notifications, modal and other functions defined in core.ts

I previously refactored many functions related to UI from core.ts to the UI directory. Roughly, each file represents a UI screen.
This is a typical example of a function that requires some dependencies. I have come to the pattern of passing callback functions in a deps object, representing dependencies. They do not need to be the exact functions defined in core.ts, but as much as the refactored function needs (like this).

I think this requires less change of current architecture, decreases access to stateful global objects, is easier to test and the function signature shows clearly all its dependencies.

What do you think?

[BassemHalim]

Hi @hatemhosny
No worries

I was initially passing the required dependencies but the dependencies kept growing ( I don't recall the number but I got to at least 10) and many of them were the global variables so I was thinking of moving the most used global variables like eventsManager, config, notifications etc. to their own modules to reduce the number or dependencies that need to be passed and reduce the number of global variables in general.

But I do agree that will be a lot of changes so I think I can just pass the dependencies for now and maybe refactor the global variables in the future.

P.S. I am getting a little busy lately so updates might be slow.
P.P.S. you are at 999 stars 🎉

[hatemhosny]

Thank you, @BassemHalim
Please take your time. No hurry at all.

P.P.S. you are at 999 stars 🎉

🥳🎊🎉