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:
- 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.
- 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 returnthis
so we can then chain an action likeself.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
action
s return theirElement
so they can be chained.
Components:
- 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)
andpublisher.on("header/select-nav/" + name, data => {})
. Additionally, I support a thirdbool
argument to support sending and listening for events over an optionally passed in Socket.io socket, likelet 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
- 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)
- 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
, withpublisher.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 aninsert
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)
}
}