Manifest.JS: A lightweight front-end structural framework

Question

Code review scope

My goal with this review is to receive big-picture observations and suggestions for improving the efficiency / ease of writing the front end of a web application with this basic framework. I want to focus on what it looks like it should do rather than the details of what it may or may not unintentionally do. Limit scope to a big-picture overview, which should help save time since it is a good sized chunk of code for one review.

Focus should be on speed of scalable (maintainable, restructurable) development, over-arching code patterns and code design of the resulting applications.

"Here's what it looks like you're trying to achieve, here's where you've succeeded, here's where you're lacking, or here's a big-picture modification that might make the resulting code easier to read, maintain, and faster to develop."

---

The problem Manifest.JS is meant to address

Designing single-page web apps I found two things I didn't like about ReactJS (at least for my typical project scale):

• Had to write more code than I wanted to accomplish basic things
• Was tedious to transport information through the app, you had to essentially pass a wire through the components to get information from point A to point B, which made the design feel tightly coupled and difficult to re-structure afterwards

I also sort of felt this way about other JS app frameworks I tried. So I wrote two fairly simple classes that work together to create a development pattern that I preferred. My goal was for this to let me:

1. Focus all of the process of building each individual component of an app into modular JS without having to care much about how each component is connected to the outside application.
2. Not have to go between multiple files or languages to edit JavaScript, HTML and CSS to build or maintain any one UI feature.

Loose coupling, separation of concerns, pure JS workflow, predictable project structure, easy data flow, no more features than needed. That's the goal, whether this code achieves it or not, I'm not sure yet.

Note: JSX sort of does some of #2, but having the two languages in one file felt a bit odd to me, I wanted my files to be a uniform language rather than JSX woven through it like with React.

---

Self critiques:

So far some self-critiques I've considered:

• When it comes to modularizing a set of Elements into a class, I could provide a single, set way of doing it so there's a clear path forward for the developer and no freedom to develop accidental anti-patterns when deciding how to package the components into modular files.

• Chaining is great. I should update .use to return this so we can then chain an action like

  self.append(new InfoPage().use(subPage, { /* properties */ }).actions.select(true))

  Create the InfoPage, use the subPage template, pass unique properties, and select it by default. Also can make actions return their Element so they can be chained.

---

Components:

1. Publisher.js - a simple message-passing class to implement the Pub Sub pattern because I wanted to be able to send namespace-separated events from any location in the app and read them anywhere else, like: publisher.emit("header/select-nav/home", this) and publisher.on("header/select-nav/" + name, data => {}). Additionally, I support a third bool argument to support sending and listening for events over an optionally passed in Socket.io socket, like let publisher = new Publisher(io()), so I could handle local and remote events in the same way.

Usage:

let publisher = new Publisher(io()) // or let publisher = new Publisher()
publisher.on("namespace1/subnamespace2/event-name", data => {}, false)
// third arg set to true tells the event handler to listen for Socket.io events

2. Element.js - a wrapper for HTML elements that facilitates the entirety of the app's HTML generation and logic, so the logic associated with each visible component of the app is tightly coupled to it, while all the components individually remain loosely coupled to each-other. I also plan to maybe add support for generating CSS classes locally within each component too.

Usage:

new Element("div", {          // create a div
  id: "header",               // set the id
  traits: { publisher },      // assign data that will be inherited by any children
  data: { name: "primary" },  // assign data to this element only
  text: "My header text",     // set text
  innerHTML: false,           // set innerHTML
  classes: [],                // assign classes
  attributes: {               // assign attributes
    "contenteditable": "true"
  },
  styles: {},                 // assign styles
  actions: {                  // create actions, which will be wrapped in a
    show: (self, arg1) => {       // wrapper func and receive the Element as
      self.clearStyle("opacity")  // their first argument, followed by any
      self.addClass("visible")    // args passed with Element.action.name()
      console.log("called with Element.actions.show(arg1)")
    },                            
    hide: self => {
      self.removeClass("visible") // remove them
      self.style("opacity", 0)    // or set styles
    }
  },
  callback: self => { // trigger as soon as the element is created
    self.append(new Element("div", {
      id: "important-icon",
      classes: ["hidden", "header-icon"],
      actions: {
        select: () => {
          self.addClass("visible")
          self.removeClass("hidden")
          self.updateText("Selected") // update text
        }
      },
      ready: self => {
        self.on("mouseover", evt => { // handle DOM events
          self.addClass("highlight")
        })
      }
    })) 
  }, 
  ready: self => { // trigger after the element is appended to a parent
    self.traits.publisher.on("events/header/" + self.data.name, data => {
      self.select("#important-icon").actions.select(); 
        // you could of course apply this listener to the icon itself, 
        // but the select feature is convenient in some cases
    })                                    
  }
}).appendTo(document.body)

3. Controller.js - validation of input during data flow grows more and more important the larger an application becomes. So it should be a choice of course, whether you want to use it, and I made it available and supported for validating the data flow both within the Element and in the Publisher. I didn't code in publisher support yet but it'll work the same at Element, with publisher.use(controller). But I also wanted a pass to pass a blueprint input to a set of elements requiring the same properties, and it makes sense for a Controller to be able to override the current input passing through it for ease of testing / debugging, so I added an insert method to it, which (as you'll see in the code) can and should be used for templating Element properties.

Usage:

let page = new Controller({
  data: data => { // pass a function to validate data however you want
    if (!data.name) return false
    else return true
  },
  traits: true, // pass true to simply ensure a setting is passed
  actions: "object", // pass a string to test against typeof
}).insert({ // and insert specific default data
  traits: {
    publisher
  },
  actions: {
    select: self => {
      let target = "header/select-nav/" + self.data.name.toLowerCase()
      self.traits.publisher.emit(target, this)
      self.addClass("visible")
    }
  },
  ready: self => {
    self.traits.publisher.emit("header/add-nav", self)
  }
});

Element.js:

import Controller from "/js/classes/controller.js"

function isCyclic(obj) {
  var seenObjects = [];

  function detect(obj) {
    if (obj && typeof obj === 'object') {
      if (seenObjects.indexOf(obj) !== -1) {
        return true;
      }
      seenObjects.push(obj);
      for (var key in obj) {
        if (obj.hasOwnProperty(key) && detect(obj[key])) {
          //console.log(obj, 'cycle at ' + key);
          return true;
        }
      }
    }
    return false;
  }

  return detect(obj);
}

function isObject(item) {
  return item && typeof item === 'object' && !Array.isArray(item);
}

function isIterable(item) {
  let type = false;
  if (isObject(item)) type = 'obj';
  else if (Array.isArray(item)) type = 'arr';
  return type;
}

function mergeDeeper(source, target) {
  let allProps = [];
  let sourceProps;
  let type;
  let targetProps;
  if (isObject(source)) {
    sourceProps = Object.keys(source);
    type = 'obj';
  } else if (Array.isArray(source)) {
    sourceProps = source;
    type = 'arr';
  } else {
    return source;
  }
  if (isObject(target)) {
    targetProps = Object.keys(target);
  } else if (Array.isArray(target)) {
    targetProps = target;
  } else {
    debugger
    throw "target missing"
  }
  sourceProps.forEach(prop => {
    allProps.push(prop);
  });
  targetProps.forEach(prop => {
    allProps.push(prop);
  });
  allProps = [...new Set(allProps)];
  let merged
  if (type == 'obj') {
    merged = {};
  } else if (type == 'arr') {
    merged = [];
  }
  allProps.forEach(prop => {
    if (type == "obj") {
      if (source[prop]) {
        if (isIterable(source[prop])) {
          if (isIterable(target[prop])) {
            merged[prop] = mergeDeeper(source[prop], target[prop])
          } else merged[prop] = source[prop]
        } else {
          merged[prop] = source[prop]
        }
      } else {
        if (source[prop] !== undefined) {
          merged[prop] = source[prop]
        } else {
          merged[prop] = target[prop]
        }
      }
    } else {
      let iterable = isIterable(prop);
      if (iterable) {
        let filler
        if (iterable == "obj") filler = {};
        else if (iterable == "arr") filler = [];
        merged.push(mergeDeeper(prop, filler))
      } else {
        merged.push(prop)
      }
    }
  })
  return merged;
}

const collectChildSelectors = (elementWrapper, selectors) => {
  elementWrapper.children.forEach(childWrapper => {
    if (childWrapper.element.id) {
      selectors[childWrapper.element.id] = childWrapper
    }
    if (childWrapper.selector) {
      selectors[childWrapper.selector] = childWrapper
    }
    collectChildSelectors(childWrapper, selectors)
  })
}

const applySettings = function(newSettings) {
  if (!newSettings) throw "bad settings"
  let settings = mergeDeeper(newSettings, {
    text: false,
    innerHTML: false,
    classes: [],
    actions: {},
    data: {},
    attributes: {},
    styles: {},
    traits: {},
    id: false,
    callback: false,
    ready: false,
  });
  if (settings.id) {
    this.element.id = settings.id
    this.selector = settings.id
  }
  if (settings.text) this.element.textContent = settings.text
  if (settings.innerHTML) this.element.innerHTML = settings.innerHTML
  if (settings.selector) {
    this.selector = settings.selector
    this.selectors[settings.selector] = this;
  }
  settings.classes.forEach(className => this.element.classList.add(className))
  Object.keys(settings.attributes).forEach(attributeName => 
      this.element.setAttribute(attributeName,
          settings.attributes[attributeName]))
  Object.keys(settings.styles).forEach(styleName => 
      this.element.style[styleName] = settings.styles[styleName])
  Object.keys(settings.actions).forEach(actionName => 
      this.actions[actionName] = () => settings.actions[actionName](this))
  Object.keys(settings.data).forEach(propertyName => 
      this.data[propertyName] = settings.data[propertyName])
  Object.keys(settings.traits).forEach(propertyName => 
      this.traits[propertyName] = settings.traits[propertyName])
  if (settings.ready) this.ready = settings.ready
  if (settings.callback) settings.callback(this);
}

export default class {
  constructor(tag, settings) {
    this.children = [];
    this.data = {}
    this.actions = {}
    this.traits = {}
    this.selectors = {}
    this.element = document.createElement(tag)
    applySettings.apply(this, [settings])
  }
  use(arg1, arg2) {
    if (arg1 instanceof Controller) {
      let controller = arg1;
      let settings = arg2;
      let mergedSettings = mergeDeeper(settings, controller.insertions);
      controller.test(mergedSettings);
      applySettings.apply(this, [mergedSettings])
    } else if (arguments.length === 1) {
      let settings = arg1;
      applySettings.apply(this, [settings])
    } else {
      throw "bad settings passed to Element"
    }
    return this;
  }
  addEventListener(event, func) {
    this.element.addEventListener(event, func)
  }
  delete() {
    this.parent.removeChild(this.element)
  }
  style(styleName, value) {
    this.element.style[styleName] = value
  }
  clearStyle(styleName) {
    this.element.style[styleName] = ""
  }
  updateText(text) {
    this.element.textContent = text
  }
  updateAttribute(attributeName, attributeContent) {
    this.element.setAttribute(attributeName, attributeContent)
  }
  addClass(className) {
    this.element.classList.add(className)
  }
  removeClass(className) {
    this.element.classList.remove(className)
  }
  on(evt, func) {
    this.element.addEventListener(evt, func)
  }
  select(id) {
    let parts = id.split("#")
    let selector = parts[parts.length - 1];
    if (!this.selectors[selector]) debugger; 
    //throw "bad selector " + selector
    return this.selectors[selector]
  }
  appendTo(elementWrapper) {
    let element
    if (elementWrapper.nodeName) element = elementWrapper
    else {
      element = elementWrapper.element
      this.parent = element
      collectChildSelectors(this, elementWrapper.selectors)
      Object.keys(elementWrapper.traits).forEach(propertyName => 
          this.traits[propertyName] = elementWrapper.traits[propertyName])
    }
    if (this.ready) this.ready(this)
    element.appendChild(this.element)
    return this
  }
  append(elementWrapper) {
    let element
    let wrapped = false
    if (elementWrapper.nodeName) element = elementWrapper
    else {
      wrapped = true
      element = elementWrapper.element
      element.parent = this
      if (element.id) this.selectors[element.id] = elementWrapper
      if (elementWrapper.selector) 
          this.selectors[elementWrapper.selector] = elementWrapper
      this.children.push(elementWrapper)
      collectChildSelectors(elementWrapper, this.selectors)
      Object.keys(this.traits).forEach(propertyName => 
          elementWrapper.traits[propertyName] = this.traits[propertyName])
    }
    if (elementWrapper.ready) elementWrapper.ready(elementWrapper)
    this.element.appendChild(element)
    if (wrapped) return elementWrapper
  }
}

Controller.js:

export default class {
  constructor(settings) {
    this.tests = {};
    Object.keys(settings).forEach(key => {
      let val = settings[key];
      if (typeof val == "boolean") {
        this.tests[key] = input => {
          return input !== undefined
        }
      } else if (typeof val == "string") {
        this.tests[key] = input => {
          return typeof input === val
        }
      } else if (typeof val == "function") {
        this.tests[key] = val;
      }
    })
  }
  test(obj) {
    Object.keys(obj).forEach(key => {
      if (!this.tests[key] || !this.tests[key](obj[key])) {
        console.log("Controller test failed");
        debugger;
      }
    });
  }
  insert(insertion) {
    this.insertions = insertion;
    return this;
  }
}

Publisher.js

export default class {
  constructor(socket) {
    if (socket) this.socket = socket;
    this.events = {};
  }
  on(command, func, socket = false) {
    if (!this.events[command]) this.events[command] = [];
    this.events[command].push(func);
    if (socket && this.socket) socket.on(command, func);
  }
  emit(command, data = {}, socket = false) {
    if (this.events[command]) {
      this.events[command].forEach(func => func(data));
    }
    if (socket && this.socket) socket.emit(command, data);
  }
}

---

Implementation

app.js:

import Publisher from "/js/classes/publisher.js"
import Controller from "/js/classes/controller.js"

let publisher = new Publisher(io())

import Header       from "/js/classes/header/header.js"
import Home         from "/js/classes/pages/home/home.js"
import News         from "/js/classes/pages/news/news.js"
import Leaderboard  from "/js/classes/pages/leaderboard/leaderboard.js"
import Account      from "/js/classes/pages/account/account.js"
import Builder      from "/js/classes/pages/builder/builder.js"

let header = new Header(publisher)

let page = new Controller({
  data: true,     // () => { } // validate the value however you choose
  traits: true,   // It's good to have this capability for debugging
  actions: true,  // or for if your boss wants all your data interfaces
  ready: true     // validated because he read it in a hip dev blog
}).insert({       // <- But insertion is the feature you'll be using
  traits: {       // more often to test input data, debug, and like with
    publisher     // this case, apply a single input object to multiple
  },              // Elements
  actions: {
    select: self => {
      let target = "header/select-nav/" + self.data.name.toLowerCase()
      self.traits.publisher.emit(target, this)
      self.addClass("visible")
    }
  },
  ready: self => {
    self.traits.publisher.emit("header/add-nav", self)
  }
});

new Home().use(page, {
  data: {
    name:       "Home",
    iconPath:   "/assets/home/home-1.png",
    cornerPath: "/assets/corners/corner-1.png",
  }
}).appendTo(document.body)

new News().use(page, {
  data: {
    name:       "News",
    iconPath:   "/assets/news/news-1.png",
    cornerPath: "/assets/corners/corner-5.png"
  }
}).appendTo(document.body)

new Leaderboard().use(page, {
  data: {
    name:       "Leaderboard",
    iconPath:   "/assets/leaderboard/leaderboard-1.png",
    cornerPath: "/assets/corners/corner-3.png",
  }
}).appendTo(document.body)

new Account().use(page, {
  data: {
    name:       "Account",
    iconPath:   "./assets/profile/profile-1.png",
    cornerPath: "/assets/corners/corner-4.png",
  }
}).appendTo(document.body)

new Builder().use(page, {
  data: {
    name:       "Builder",
    iconPath:   "./assets/builder/builder-1.png",
    cornerPath: "/assets/corners/corner-2.png",
  }
}).appendTo(document.body).actions.select()

/js/classes/pages/builder/builder.js:

Here I used a sort of odd return statement in the constructor, purely for visual purposes because I like using new ModuleName() in the file where it's used, as opposed to a function call, but you can do it either way.

import Element from "/js/classes/element.js"
import NavBar from "/js/classes/pages/builder/nav-bar.js"
export default class {
  constructor() {
    return new Element("div", {
      id: "builder",
      classes: ["page"],
      actions: {
        select: self => {
          let target = "header/select-nav/" + self.data.name.toLowerCase()
          self.traits.publisher.emit(target, this)
          self.addClass("visible")
        }
      },
      ready: self => {
        self.traits.publisher.emit("header/add-nav", self)
        self.actions.select()
      },
      callback: self => {
        self.append(new NavBar());
        // add more elements
      }
    })
  }
}

/js/classes/pages/header/header.js

import Element from "/js/classes/element.js"
import NavIcon from "./header-nav-icon.js"
export default class {
  constructor(publisher) {
    return new Element("div", {
      id: "header",
      traits: { publisher },
      ready: self => {
        self.append(new Element("div", {
          selector: "title-wrapper",
          classes: ["title-wrapper"],
          ready: self => {
            self.append(new Element("div", {
              selector: "location-wrapper",
              classes: ["location-wrapper"],
              ready: self => {
                self.traits.publisher.on("header/add-nav", data => {
                  self.append(new Element("div", {
                    selector: "location-item-wrapper",
                    classes: ["location-item-wrapper"],
                    ready: self => {
                      self.traits.publisher.on("header/select-nav/" + 
                        data.data.name.toLowerCase(), data => {
                        self.addClass("visible")
                      });
                      self.append(new Element("div", {
                        id: data.data.name.toLowerCase() + "-nav",
                        classes: ["location-item", "heading"],
                        text: data.data.name
                      }))
                      self.append(new Element("img", {
                        classes: ["location-item-icon"],
                        attributes: {
                          "src": data.data.iconPath.split(".png")[0] + "-flat.png"
                        }
                      }))
                      self.append(new Element("img", {
                        selector: "corner",
                        classes: ["corner"],
                        attributes: {
                          "src": data.data.cornerPath
                        }
                      }))
                    }
                  }))
                })
              }
            }))
            self.append(new Element("div", {
              selector: "sub-location-wrapper",
              classes: ["sub-location-wrapper", "subheading"]
            }))
          }
        }))
        self.append(new Element("div", {
          selector: "nav-wrapper",
          classes: ["nav-wrapper", "center-contents"],
          ready: self => {
            self.traits.publisher.on("header/add-nav", data => {
              console.log("header/add-nav, data", data.data)
              console.log("adding nav-item")
              self.append(new NavIcon().use({
                data: data.data
              }))
            });
            self.append(new Element("div", {
              classes: ["title-bg-wrapper"],
              ready: self => {
                self.append(new Element("img", {
                  classes: ["title-bg-icon"],
                  attributes: {
                    "src": "./assets/header.png"
                  }
                }))
                self.append(new Element("div", {
                  classes: ["title-bg-text"],
                  innerHTML: "BIDRATE <br/> RENAISSANCE"
                }))
              }
            }))
          }
        }))
      }
    }).appendTo(document.body)
  }
}

Answer 1

From a short review;

• isCyclic -> I would consider throwing obj into JSON.stringify and catch the relevant exception

• function detect is not a great name, it's okay because of the context, but could be better

• //console.log(obj, 'cycle at ' + key); <- bad comment

• The code is using both var and set and const, there is true value in analyzing code and using only set/const

• function isObject(item) <- an icky name since really you check whether it's an object but not an Array (which is also an object), hence why you could not use this function in if (obj && typeof obj === 'object')

• function isIterable(item) { <- very icky name, the reader assumes it returns a boolean, especially with the first line being false but then you also return obj or arr, perhaps call it iterableType that return undefined, 'obj', or 'arr'?

• You are skipping on curly braces in isIterable, you should not

• debugger does not belong in production code

• this

  sourceProps.forEach(prop => {
    allProps.push(prop);
  });
  targetProps.forEach(prop => {
    allProps.push(prop);
  });
  

  could be

  allProps = allProps.concat(sourceProps).concat(targetProps);
  

• You know that only Object and Array are iterable, and that the property is iterable so

  let filler
  if (iterable == "obj") filler = {};
    else if (iterable == "arr") filler = [];
  

  can be

  let filler = iterable=="obj"?{}:[];
  

• On the whole I would read up on the ternary operator, this

    if (source[prop] !== undefined) {
      merged[prop] = source[prop]
    } else {
      merged[prop] = target[prop]
    }
  

  could be shorted and more readable (to me);

   merged[prop] = source[prop]?source[prop]:target[prop];
  

  and in this case it could even be shortened down to

   merged[prop] = source[prop] || target[prop];
  

• The code has inconsistent use of semicolons, very annoying to read

• You should pick a naming/coding standard and stick to it, before this the function keyword was used, and now the code switches to this;

     const collectChildSelectors = (elementWrapper, selectors) => {
  

• Not sure why you are not providing all the possible parameters to addEventListener

     addEventListener(event, func) {
              this.element.addEventListener(event, func)
     }
  

• You do the below 5 times with different parameters, this could use a helper function to make this more readable;

   Object.keys(settings.styles).forEach(styleName => 
           this.element.style[styleName] = settings.styles[styleName])
  

Answer 2

Big picture overview

Limit scope to a big-picture overview, which should help save time since it is a good sized chunk of code for one review.

While I plugged the modules into a plunker, it is still difficult for me to determine if the framework would help me with a SPA with limited code. I see a lot of methods that accept self as the first (and often only) parameter. Why can't they operate on this? Is the context not bound correctly for that?

I created an event emitter module for an interview. The requirements sound like the Pub-Sub pattern and I implemented similar methods as the Publisher. The requirements called for a way to have a "one-time" handler, as well as a way to un-register a registered handler function. You might consider offering such functionality with your publisher module.

The bottom line is: if you feel like this framework allows you to write less code than you otherwise might with many other frameworks then go ahead and use it.

Targeted JS feedback

I noticed the const keyword only appears in your code twice - for two function expressions i.e. collectChildSelectors and applySettings. It is recommended that const be the default keyword for all variables, and then if re-assignment is necessary, switch to using let. Also, avoid var, unless there is a need for something like a global variable but that is also frowned upon.

Some parts of the code use === for comparing values but others use ==. A recommended practice is always using strict type comparison .

For readability, use a consistent quote style for string literals- either single or double quotes but not both.

mergeDeeper() could use spread operator instead of forEach() -> push for sourceProps and targetProps

allProps.push(...sourceProps, ...targetProps)

Name of function isIterable seems somewhat strange given that it can return a string or boolean. Maybe a more appropriate name would be iterableType - even if it returns false then the caller would know the value is not iterable.