-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add NodeJS compatibility, use getRawTextContent #33
Conversation
Interesting! Is it only about loading or can you parse html in node too? |
It's primarily to complete the loading, ready for a polyfill that makes hickory able to parse html in nodejs. |
Is this something that is useful by itself, if you still can't parse the HTML? |
With the commit hickory.core/parse works in nodejs after setting js/DOMParser to one of the available DOM parsers available for node. I currently use xmldom:
For full use of hickory I also have a hairy polyfill to accommodate that hickory extends NodeList instead of using standard dom to iterate over childnodes and attributes. A minor additional change to hickory would eliminate the need for this patch. But making hickory loadable on nodejs has value in itself, as it doesn't become a showstopper for using other modules that depends on hickory. Case in point is kioo. |
Ok, I don't understand the nodelist issue (or most of what you said really), but I think we don't want to stop anyone's show. Can you explain what the further fix that obviates this patch is? |
Sorry for not explaining more clearly. The problem for compatibility with node is that hickory.core (cljs) assumes that there is a The quick fix is a wrapper that coerces all dom nodelists into a sequence, for example using a function like:
Then use
Or perhaps as-seq should just integrate the aget? Your choice. |
With these commits, hickory can be used on nodejs to parse markup into hickory or hiccup format, after setting js/DOMParser to one of the available DOM parsers available for node, such as:
|
Can we not simply use |
We can indeed simply use
|
I don't think we need to keep this compatibility. I don't remember exactly why I choose this option when porting to ClojureScript but in retrospect it looks unnecessarily complex.
Can this happen? My opinion is we should just go for the simple case. Do you mind modifying the PR to achieve that? |
Yes, nodes from I'll modify the PR for the simple case, eliminating the |
Right that may happen. I do think it is considered bad practice to do this. |
I'd expect the |
Extending |
Good catch! Looks great now, probably ready to be merged. @davidsantiago what do you think? |
I'm fine with an API break and issuing a new minor version, if that's the right thing to do. I'd rather just solve it and avoid any associated complexity. If you're happy with the patch, I'd just like someone to give me a little snippet for the readme change notes so I can be sure I am explaining what changes. |
There's no real API breakage beyond the Tentative change notes:
|
More accurate to say: text nodes were repeated as the issue could cause text nodes to be parsed and repeated multiple times.
Consider instead saying no longer extended to be seqable. |
Update for nodejs compatibility PR
I am now looking into node more for Macchiato. What's the status of this PR? |
Yep, sorry about that, this one fell off my radar as other things have been placed above it on my stack. I'll try to find some time tonight to get this merged in. |
Is there help needed to get this released? |
Yes, very much. I don't know javascript or node or clojurescript really, and there are a number of issues and pull requests pending that I simply don't understand. There has been some discussion and debate in some of them that I don't understand. And just in a more general sense I don't understand what node support entails or what's missing or when it would be done. I have been meaning to do some studying and figure out what is going on in them, but I simply don't have the time to get up to speed for various life reasons. If you can understand what is going on in pending node PRs and issues and figure out what is needed and what is ready to merge in, I'm going to defer to anyone who knows what they are doing on this environment and can help me figure out what to put in, what changes to make, and what to leave out (if anything). |
I would love it to get this working. (nodejs/enable-util-print!) (set! js/DOMParser (.-DOMParser (cljs.nodejs/require "xmldom"))) (defn -main [] (set! main-cli-fn -main)` But when I run this
` |
Likely xmldom requires well-formed XML markup, with each start tag having a matching end tag:
If it still fails, I suggest you trim the markup down to a single element:
As you're new to Clojure, have you actually gone through the extra steps of installing the "node" branch of Hickory rather than just using a dependency on the stock version that doesn't yet support node? If so, I award you the coveted lein install badge, please use your new powers with care. |
Very nice patch! I got https://github.com/mpcarolin/markdown-to-hiccup working in nodejs (via lumo 1.8) by using this fork, installing
|
Hi folks, I'm trying to get Hickory to work on NodeJS courtesy of this PR/branch.. However, with the following code (at the repl):
I get the following error:
This is with The following equivalent Clojure code works fine:
What am I doing wrong? Any ideas? Thanks! |
The likely cause for the
A work-around is to patch the Attribute constant to be consistent with jsdom:
A more permanent fix is for PS: Be careful setting
|
Hi @njordhov thanks for the help!
|
By setting hickory.core/Attribute to nil (clj-commons/hickory#33 (comment)) Code restructure to isolate side effects into 'app' layer functions Use with-redefs instead of set!
@slipset Any thoughts on if this PR is ok to land? Looks like the main feedback was addressed and then approved at #33 (comment). Encountered same failure as #17 |
@slipset Needed to merge this before we can move on to running tests on Node.js without a browser. If tests fail we can revert. |
Issue #17 reports an incompatibility with nodejs where loading fails due to missing DOM bindings.
I have resolved this issue by replacing
js/Node
withgoog.dom/NodeType
and madejs/NodeList
optional, which eliminates the dependency on browser dom, allowing hickory to load also on nodejs.