Adds Node-compatible protocolForURL #437
Conversation
|
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. 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. |
| } | ||
| // In Node.js environments, we can't rely on the DOM correctly parsing URLs. | ||
| // Instead, we'll fall back to Node's built-in URL parsing. | ||
| if (typeof process === 'object' && process + '' === '[object process]') { |
There was a problem hiding this comment.
does this work in node-webkit?
There was a problem hiding this comment.
Unknown, but looks like this might be a better test: https://github.com/jnath/nwjs-video-player/blob/159c0d487159846664782979d46bbd61857fb429/src/app/js/nw.js#L1
There was a problem hiding this comment.
process is likely not something you want to expose on the sandbox context just for an is node check.
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. |
|
seems good! 👍 |
Basically EOL at this point, not sure it's worth anyone putting effort in here that could be put into glimmer. |
@tomdale c |
tomdale
force-pushed
the
node-protocol-for-url
branch
from
68cf674 to
38019b6
Compare
|
@stefanpenner @krisselden @chancancode Replaced detecting the environment with a feature detection. How does this approach look? |
|
@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? |
| // Otherwise, we need to fall back to our own URL parsing. | ||
| // Global `require` is shadowed by Ember's loader so we have to use the fully | ||
| // qualified `module.require`. | ||
| URL = module.require('url'); |
There was a problem hiding this comment.
what if there is no require? Might be nice to fail fast then (might be aligned with my comment bellow about doing this before setting up the prototype.
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 URL parsing doesn’t happen when setting the `href` property on an `A` element and falls back to using Node’s `URL` package.
tomdale
force-pushed
the
node-protocol-for-url
branch
from
38019b6 to
7f7ceeb
Compare
|
@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. |
When running in the browser, we rely on the browser’s built-in HTML
parsing for cases when users provide unescaped HTML content as dynamic
values in their templates (i.e., `{{{unsafeHTML}}}`). Because the final
output is a DOM tree, we ask the browser to parse the HTML and turn it
into a document fragment, then insert into the appropriate location in
the rendered element.
However, for server-side rendering, the final output is serialized HTML,
not a DOM tree. In the case of raw HTML provided by the user, we’d be
doing extra work to go from raw HTML -> parsed DOM -> serialized HTML.
(That we’d need an HTML parser also adds complexity.)
This patch allows us to detect an extension to the DOM API we’ve added
to SimpleDOM that allows us to insert a node in the DOM tree that means
“raw HTML” (similar to CDATA). When we go to serialize the DOM tree, any
raw HTML nodes are written as the HTML that was provided when created.
tomdale
force-pushed
the
node-protocol-for-url
branch
from
ec88eed to
e92d8c9
Compare
|
Gonna merge this, @stefanpenner let me know if you've got any issues and I will fix. |
Adds Node-compatible protocolForURL
The current implementation of the DOM helper’s
protocolForURLrelieson the implicit parsing and normalization that the browser’s DOM does
when setting the A element’s
hrefproperty.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