search

tildeio / htmlbars

A variant of Handlebars that emits DOM and allows you to write helpers that manipulate live DOM nodes
MIT License
1.6k stars 193 forks source link

Adds Node-compatible protocolForURL #437

Closed tomdale closed 8 years ago

tomdale commented 8 years ago

The current implementation of the DOM helper’s protocolForURL relies on the implicit parsing and normalization that the browser’s DOM does when setting the A element’s href property.

However, the DOM helper is often used with subset DOM implementations like simple-dom. In those cases, we cannot rely on this normalization.

This commit detects when the DOM Helper is running in Node and uses the built-in URL package to parse the protocol instead.

/cc @wycats @chancancode @rwjblue @stefanpenner

tomdale commented 8 years ago

It looks like CJS commits were disabled in https://github.com/tildeio/htmlbars/commit/0c51a428dd0aa8358c075d73d82fd90df9d6536f but no rationale for why was provided in that commit. Presumably it was for a good reason, but it means I couldn't write a test for this case.

I think it's OK because this is implicitly tested in Ember anyway and this implementation of the DOM Helper is EOL once Glimmer2 lands.

stefanpenner commented 8 years ago

It looks like CJS commits were disabled in 0c51a42 but no rationale for why was provided in that commit. Presumably it was for a good reason, but it means I couldn't write a test for this case.

They where breaking the build, I did not have time to fix them, and we needed to get some critical work out to ember. I intended to address this, unfortunately have not had a chance.

Maybe someone else can.

chancancode commented 8 years ago

seems good! :+1:

tomdale commented 8 years ago

@stefanpenner

Maybe someone else can

Basically EOL at this point, not sure it's worth anyone putting effort in here that could be put into glimmer.

stefanpenner commented 8 years ago

Basically EOL at this point, not sure it's worth anyone putting effort in here that could be put into glimmer.

@tomdale c

tomdale commented 8 years ago

@stefanpenner @krisselden @chancancode Replaced detecting the environment with a feature detection. How does this approach look?

stefanpenner commented 8 years ago

@tomdale I would feel better doing the test before the prototype member is set, rather then replacing the SharedFunction during invocation. Is this possible, or is it lazy fora specific reason?

tomdale commented 8 years ago

@chancancode @stefanpenner I updated the implementation to install the methods on the instance, and also added support for raw HTML sections. That last change let's us remove all of the monkeypatches from Ember/FastBoot.

tomdale commented 8 years ago

Gonna merge this, @stefanpenner let me know if you've got any issues and I will fix.