Skip to content
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

NodeJS 10.x: use libxmljs 0.19.3 for libxmljs/libxmljs#523 #8

Closed
wants to merge 1 commit into from

Conversation

eyz
Copy link

@eyz eyz commented Aug 27, 2018

@znerol, libxmljs 0.19.3 provides support for NodeJS 10.x; see libxmljs/libxmljs#523
I am using this xmlshim NPM library as a dependency pulled in from another NPM library (node-blockly), and git://github.com/znerol/libxmljs.git#xmlwriter-0.18.0 does not allowing building under NodeJS 10.x, at least in my environment.

Please let me know if you need any steps to reproduce. It should be as straightforward as trying to npm install xmlshim under NodeJS 10.x

@eyz
Copy link
Author

eyz commented Aug 27, 2018

@znerol it looks like the Travis CI build failed for this PR, so instead, could you perhaps merge in the latest libxmljs commits (up to 0.19.3 at least) to your libxmljs fork? It looks like you have some changes in your fork to support passing the Travis build. I believe it's only this bump of node-pre-gyp that's actually needed @ libxmljs/libxmljs@b43e577, but I suspect you'd have merge conflicts in the future if that was the only change made to resolve this, rather than merging in from upstream libxmljs. Thanks for any consideration!

@eyz eyz changed the title use libxmljs 0.19.3 for libxmljs/libxmljs#523 NodeJS 10.x: use libxmljs 0.19.3 for libxmljs/libxmljs#523 Aug 27, 2018
@richtera
Copy link

When will this be finalized? I need to use 9.11 instead of 10 but now other dependencies are incompatible with 9: error graphql@14.0.0: The engine "node" is incompatible with this module. Expected version "6.x || 8.x || >= 10.x".

@eyz
Copy link
Author

eyz commented Aug 31, 2018

@richtera are you also using node-xmlshim (aka xmlshim on NPM), or only libxmljs? (regarding your comment on the linked libxmljs PR) Also, when will what be finalized, this xmlshim PR?

xmlshim has a dependency on libxmljs. I suspect libxmljs isn't dependent on xmlshim.

It sounds like you are dependent on libxmljs, and if you are not dependent on node-xmlshim, then this PR is almost certainly not applicable to you, and is only associated because I mentioned the PR from libxmljs.

@richtera
Copy link

richtera commented Sep 1, 2018

I am installing node-blockly which pulls it all in. Let me try to add a resolution section into my package.json file to pick this version. With node 9.11 it just works.

@eyz
Copy link
Author

eyz commented Sep 1, 2018

@richtera I had the same issue, which is why I submitted this PR. You can (temporarily) use my fork of node-blockly via eyz/node-blockly@3dd2524 (or fork it to your own Github repo), which only has a modified package.json to use my fork of node-xmlshim (this PR we're commenting on), instead of znerol/node-xmlshim 0.2.1 -- again, forking to your own repo if required, and then referencing from your own fork of node-blockly if required. Perhaps there is a better way, but this is the way I solved it.

Here's the diff in my project's package.json (which uses node-blockly too) to use the above changes -

-    "node-blockly": "*",
+    "node-blockly": "git://github.com/eyz/node-blockly.git#3dd2524c1b222cbb00817266f12a40e5a1654cef",

^ you can install NPM packages directly from Github repos (such as above) in package.json; they don't have to be published on NPM, which is how I ended up resolving this in the meantime, prior to this PR (that we're commenting on) being evaluated

As far as I can tell, the only reason the checks are failing on this PR (that we're commenting on) is because there are Travis integrations for this project that are not in the upstream repo for libxmljs. My project's blockly works fine on NodeJS 10.x with the above change, to use the two chained package.json changes in my forks of the respective projects to ultimately use libxmljs 0.19.3, which has the fix for NodeJS 10.x

@ArfyFR
Copy link

ArfyFR commented Oct 8, 2018

Hi, also waiting for this update for node-blockly (which has libxmljs as dependency via xmlshim and ### npm i node-blockly failing due to too old libxmljs@0.18.0)

@dmythro
Copy link

dmythro commented Dec 28, 2018

Any news on this?

@znerol
Copy link
Owner

znerol commented Dec 28, 2018

I'm really sorry for this situation. xmlshim is relying on functionality which is not yet merged into the upstream libxmljs branch. See libxmljs/libxmljs#493 for the context.

@znerol
Copy link
Owner

znerol commented Jan 26, 2019

Fixed with version 0.2.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants