search

complate / complate-stream

complate's core library for rendering HTML via JSX
https://complate.org
6 stars 7 forks source link

introduced logging to avoid exceptions for HTML validation #6

Closed FND closed 6 years ago

FND commented 6 years ago

introduced logging to avoid exceptions for HTML validation

the latter were deemed excessively disruptive for real-world scenarios

caveats:

  • not entirely sure whether "warning" would be preferable to "error" for this kind of validation
  • relying on console.log as default might be problematic in some environments - perhaps a no-op would be more suitable here?
  • due to the largely functional approach, a log function is passed into element generators (and then passed through to generateAttributes), which might not be entirely intuitive however, this actually makes element generators' signature more readable by avoiding a non-descriptive boolean argument
  • element generators' more complex signature might negatively impact performance (though there are other potential hot spots already; such concerns should be addressed via proper profiling instead)

1311e58c9f41e61bea6da4c76366ed1cf53b0244

this should also make optional ID validation a little easier

mechanoid commented 6 years ago

Overall, LGTM 👍. Whether it should be a plain log or an error log statement I think we should decide later. This may be more obvious with actual demand in place.

moonglum commented 6 years ago
FND commented 6 years ago

Thanks for the feedback!

I think this is a warning, not an error.

I see your point, and indeed I'm not entirely sure myself: OTOH it's invalid HTML, but OTOH HTML is intended to be fault-tolerant. And yet, an error log message is not an exception, so ... I remain uncertain. 😕 (I like @mechanoid's suggestion, but not sure whether that'd be a cop-out.)

Shouldn't the simpleLog function be injected by the execution environment?

You mean like global.simpleLog? To what end? (Note that simpleLog is just the default value for the log function - which would typically be passed into the root element generator to kick off rendering.

I think performance is a topic we should discuss in general.

Yes, indeed. However, that depends in large part on the respective adaptor (e.g. V8 vs. Nashorn, but also efficiency WRT things like the JavaScript engine's persistence vs. evaluation from scratch).

I'm guessing that the respective adaptor/environment plays a much bigger role than any potential optimizations within complate-stream, so I'll choose not to worry about micro-optimizations here and now. (Also, I'm guessing that we won't have any problems competing with Haml's performance either way, so whatever we do here should be good enough.)

mechanoid commented 6 years ago

I oversaw the default-ness of the simpleLog function at first myself. I like that it is stupid and simple for those defaults. We should not overthink such things, again, without having any demand. And I don't see it as a cop-out. I think we all are happy and open for discussion once there is someone that asks.

FND commented 6 years ago

I went ahead and merged this, addressing #2 in the process - I'd still like to get consensus on the issues above (log level and, if the above didn't address it, default logger) before making a release though.

FND commented 6 years ago

67fcc1a75dcb64945112fefebe2b33394dd49b92 fixes the logger not being passed through to child elements:

this was overlooked in b6bec9d2b4a8f44103e4e47fccd1a5161b0f61f8 - which worries me a little, as it suggests that this way of passing around parameters (which is the way of functional programming) is kinda brittle

😕

On the bright side, these changes made it fairly easy to not just balk at non-empty void elements (#2 / a9844b73d24d56196c5ef7df6ad769019c803b95), but also to flag duplicate IDs (1f674aec4e3a84e72f5af5216117548abf205eb0).