MatthewVita / node-hl7-complete

Node module that is bridged with the Java Hapi HL7 library.
55 stars 12 forks source link

set java classpath once #14

Closed privateOmega closed 3 years ago

privateOmega commented 3 years ago

Earlier, class path was being set inside the NodeHL7Complete class, causing node-java to throw the following error when trying to instantiate multiple instances of NodeHL7Complete

/Users/mathew/Projects/trials/node-hl7-trial/node_modules/node-hl7-complete/index.js:15
  java.classpath = java.classpath.concat(javaClassDependencies);
                 ^

Error: Cannot set classpath after calling any other java function.
    at new NodeHL7Complete (/Users/mathew/Projects/trials/node-hl7-trial/node_modules/node-hl7-complete/index.js:15:18)
    at Object.<anonymous> (/Users/mathew/Projects/trials/node-hl7-trial/index.js:4:31)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:831:12)
    at startup (internal/bootstrap/node.js:283:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:623:3)
MatthewVita commented 3 years ago

Hi @privateOmega! I really appreciate you changing this. Code looks great too.

I pushed the latest version of the package to NPM after testing/merging your PR.

<optional> I'm interested in seeing how this project can be improved in terms of easier dependency management. Currently it's fair to say that all is well when using Java 8 and Node 8... but that's obviously less than ideal. I just added a notice on the README around why this is the case/how to install the package here.

If you have time/interest, I'd like to hear about how you deal with installation in a general sense. A bonus task would be looking into a solution where we can somehow allow for those newer java/node versions. If it's not possible, maybe including a Docker Compose file in the project would be useful.

This is a just a typical small project maintainer asking for help. If this is not of interest to you, I really appreciate your contribution and thanks! </optional>

privateOmega commented 3 years ago

@MatthewVita Thanks for merging this PR.

I see your point regarding dependency management, and I have tested this project against node 10, it works there as well, but when we switch to node 14, the following problem arises and that is because the Handle method was removed in node 12, but it technically isn't a fault of this project, just that the dependency version of node-java is behind.

error: no template named 'Handle' in namespace 'v8'

I would propose for this project to set engine param in package.json like below for the current major version and release a new major version with updated node-java and node engine, so that this works without change on latest node versions also.

{ 
  "engines" : { 
    "node" : "<11.0.0" 
  }
}

Regarding supporting newer versions of java, it technically isn't on us, its just node-java has to support the latest versions, since that project covers the bindings for java.

The idea for a docker compose file which will probably include a setup for newer version of java and node doesn't really make sense to me, maybe you could explain a bit more in detail?

Also on a different note, does upgrading to hapi v2.3 entail a lot of changes here in your opinion, since it was released around 2017, which was 4 years ahead of v2.2, probably having a lot of improvements on conversions and support for newer versions of hl7 messages as well, this is of quite an interest to me.

EDIT: I have already tried out npm i java on node 14 and it seems to replace Handle with Local, so we are covered on latest versions of node, I guess latest versions of java is the only thing remaining.

I went ahead and raised a PR #16 for the hapi version update. :D

MatthewVita commented 3 years ago

(Generic message on recent conversations: I'll be looking at your posts/PR more closely this weekend. Crazy week ahead for me!)

Hi @privateOmega,

I couldn't get it to work on Node 12 (didn't bother testing 14). It comes down to Java 8 / Node 8 for me. What OS are you using? I'm on the latest Ubuntu. This shouldn't matter, but I'd like to know for comparison. I use nvm (for Node) and Jabba (for Java) to test different versions. "engines" look promising but I'd like to test a bit.

Regarding Docker/Compose, this is basically a viable solution for those that want a single API endpoint with the package under-the-hood. While we'd be able to lock down the versions perfectly on every machine, it doesn't meet everyone's use case.

Regardless, the dockerfile would be based off of Ubuntu/Minideb and have RUN apt install openjdk-8, RUN sh https://deb.nodesource.com/setup_x.x or whatever, etc. Obviously pseudo code here. I can detail the points of compose later - not too interesting. "Everybody" loves/uses Docker and I'm sure folks have used this our project as a webservice anyway. Just another option? Interested in your thoughts. At this point, I'm looking for the best DX we can have with this. Too bad we're set back by node-java not supporting modern java.

Thanks for the Hapi PR.

privateOmega commented 3 years ago

@MatthewVita I also use nvm for node version switch, but java isn't something I work very frequently with hence I don't have jabba setup. Currently I have java v1.8. I have tried out the package with both node 12 and node 14 and works very well. I hope you did do npm i java before you tried it out, which was a point in my previous comment.

I am on macOs Mojave if that helps.

Well like you said, some people might use it, but I don't see any requests for that and the effort might not be worth it, but I guess we could always revisit the idea if someone requests for it, what do you think?

I strongly feel keeping the package dependencies updated, keeps the package interesting for all sorts of folks who can try out anything they want to.