vritant24 / cleave-html

A modular way to write static webpages
MIT License
1 stars 0 forks source link

Ast nodes #12

Closed vritant24 closed 6 years ago

vritant24 commented 6 years ago

This is what I've structured the AST nodes as. If you see any way this can be made better / done differently, please let me know. Still iffy about JS objects.

If not, I'll add some tests and type-checking.

claytn commented 6 years ago

I don't see any trouble with the current implementation so feel free to merge when ready. The only thing I am partially concerned with is that we are overwriting the prototype of each object completely.

tag.prototype = {};

I haven't done this before and don't know the consequences. The only thing I can think of is that we may be sacrificing some built in functionality on the base Object type. That being said it's unlikely we will even need Object functionality since these are AST nodes and nothing more. Do you have any further insights on this?

vritant24 commented 6 years ago

Hmmm yeah overwriting the constructor could possibly be bad. I'll rewrite it to not overwrite the prototype.

vritant24 commented 6 years ago

I added tests for the AST nodes. Anything I missed in those? Also, would type checks be needed when adding instantiating nodes and or adding children and attributes?

claytn commented 6 years ago

Tests look good. Not too sure what else would need tested in that realm. As far as typing goes we could just make a check that anything added as a child is another node type and attributes will always need to be a string. I think that may be worth adding for development purposes. If we write all portions interacting with the AST correctly we shouldn't see errors with this, but it would be possible to avoid some debugging time if we added them now.

vritant24 commented 6 years ago

yup those type checks are already in there. I'll merge then when you approve