mudgen / webscript

Webscript is a Javascript library for creating DOM elements. Use it to create web applications. It is like HTML but it is Javascript. It is designed to work with existing libraries.
https://mudgen.github.io/webscript/docs/
MIT License
86 stars 9 forks source link

Code review #3

Closed lhorie closed 4 years ago

lhorie commented 4 years ago

Hi, rather than leaving a huge PM on reddit, I figured I'd leave a code review here.

First, wanted to say the idea of leveraging existing libraries is neat. I like the todo example leveraging solid.js.

I looked a little bit at the readme and it said it supports server-side rendering, but from looking at the code, that didn't seem to be the case. Is there another file or package I missed? Or is one meant to use jsdom? Some clarity there would be helpful.

Re: the code

Other than those, I'd recommend adding a blurb to your docs about tagged string template (Not everyone knows what they are)

You may also want to think a bit about technical arguments if anyone inevitably asks about pros and cons. As I mentioned on reddit, what hyperscript has going for it is JSX compat. Since your lib is a layer on top of it, then by definition it can't be as fast, so it ideally should make up in other ways (e.g. developer experience). Bear in mind that some arguments are very opinionated - e.g. developer experience :)

mudgen commented 4 years ago

Hi Leo! Thank you for this feedback!

Webscript is meant to be used with an underlying DOM implementation, like the browser or jsdom etc. For now I removed the server-side mention in the documentation since I haven't tested it on the server side.

I think nooping booleans is a good idea. So I made that change. Thank you.

Yes, I need to deal with SVG better as you mentioned. Do you have any idea or suggestions about how to go about this? Just asking in case you know a good or realistic approach. I am very interested in developer experience.

Yes, I am aware that IE11 doesn't support proxies. I think this is a good thing in disguise because it means that I don't have to worry about the other things that IE 11 does not support which means less need for babel or transpiling which I want to get away from.

It is a good idea about adding a blurb to the docs about what tagged string templates are. I will do that. I'm doing a lot of work to improve the documentation.

Yes, I will add more documentation about the pros and cons. Thank you.

If you have other ideas, suggestions and feedback I am really happy to receive it.

mudgen commented 4 years ago

I found a way to solve the SVG problem.

  1. Webscript is designed to be used by existing libraries so it doesn't make sense for it to include its own createElement function so I removed it from webscript.js.

  2. I changed elementBuilders so that the createElement argument must be supplied by the user. The user should supply this from the UI library the user is using.

  3. I created a separate file called createelement.js that contains a simple DOM implementation of createElement and createSVGElement. These can be used if the user is not using a UI library.

  4. createSVGElement properly creates SVG elements and has support for xlink.

theSherwood commented 4 years ago

As I mentioned on reddit, what hyperscript has going for it is JSX compat. Since your lib is a layer on top of it, then by definition it can't be as fast, so it ideally should make up in other ways (e.g. developer experience).

I haven't done any benchmarking, but I can't imagine webscript would be any slower than tagged template literals, and there are a number of libraries that use those.

mudgen commented 4 years ago

I have been working on the documentation. I'm still working on it but here it is now: https://mudgen.github.io/webscript/docs/

mudgen commented 4 years ago

I added documentation about tamplate tagged literals.