MatthewVita / node-hl7-complete

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

Project Discussion (July 4th) #19

Closed MatthewVita closed 3 years ago

MatthewVita commented 3 years ago

Hi @privateOmega,

Sorry for my absence. Life keeps happening. I figured I'd put together this issue so we can discuss recent changes/ideas in one place. Please reference everything by the section name.

Node Version

I'm happy to say that I'm able to install the project with Node versions greater than 8 now (e.x.: 14). All it took it was bumping node java to 0.12.1. Please let me know if that PR is correct :).

Node Engine Version

I get the idea of incorporating this, and we should, I just don't follow why you have Node 11 as the limit. "node" : "<11.0.0". Can we go with 14 and under? I'm new to this package field/concept so forgive my ignorance!

Hapi/Jar Updates

Thanks for handling this. All is testing well on my end. It's good to know we are current and folks can benefit!

HL7 Jar Versioning Question

I figured I'd keep the Node and Java codebases versioned separately. I don't see it ask a problem honestly. Let me know if I'm missing something/this is a bad pattern.

SLF4J Question

I saw your comment. I haven't had the time to look into this. It's not a great developer experience to always see this unrelated log all of the time.

README Node Version Reference

Currently the README states "[...] rely on those versions to function (Java 8/Node 8.0.0 exactly)." Once my PR is good, I'll update the Node part.

Mention of Docker

You asked if having a Docker solution would contain a newer version of Java. This (and Node) can be locked down. The reason I brought this up is because most developers likely (?) don't use Java 8 anymore and don't want to roll it back (even with solutions such as Jabba). Having a container would solve this problem if the developer is in a Docker-centric environment and doesn't mind making a call http to actually use our solution. From my experience, this isn't an uncommon approach in that environment. I'm still considering if this is worth it. I'll put together a proof-of-concept.

Publish

I haven't forgot about publishing everything to NPM. On my list. I haven't looked into how to give you NPM access.

privateOmega commented 3 years ago

@MatthewVita I apologize for my late reply as well, been busy with work these past couple of weeks.

Node Version

PR LGTM, but I recommend we set engine field on package.json according to my following comment before we merge this PR.

Node Engine Version

Oh that <11 was for the current package of node-hl7-complete i.e anything less than < 3.1, since existing npm packages don't work with node 12 or greater. -> we should release a npm package version with the hapi version bump I had merged already.

But once we merge the node-java PR, the semver should be updated to 3.1 and node-hl7-complete will be supported on all versions of node > 7 (set by engine field of node-java).

Hapi/Jar Updates

Great to hear that man.

HL7 Jar Versioning Question

I assumed you had some correlation reasoning between the versions of node-hl7-complete and hapi.jar

SLF4J Question

This is super annoying, but I have no clue how I could help here.

README Node Version Reference

I figure it would be best to mention that anything <3.1 of node-hl7-complete will only work on node versions < 11 and for anything beyond that, you should be using node-hl7-complete@3.2

Mention of Docker

I appreciate your thinking, but like I said earlier I feel it would be best to put in effort for that, if someone raises a request for that, but then again if you could allocate some time for that, I would be happy to review it though.

Publish

Thanks.

privateOmega commented 3 years ago

Btw this is how you give someone npm publishing rights.

npm owner add <user> <your-package-name>

Unless you won't be able to get to the PRs in time, I don't really care about the publish rights.

MatthewVita commented 3 years ago

@privateOmega - sorry for the delay. I took a little trip :).

Node Version Engine

I put this together after some testing: https://github.com/MatthewVita/node-hl7-complete/pull/20 Let me know what you think. I mainly just used nvm to see that Node 11 is all good and Node 13 shows the warning.

Release

The PR that I linked above includes the node-hl7-complete bump to 3.1.0. We can polish up this branch before pushing - I'm in no hurry... want to make sure it's perfect.

README Dependency References

Also in that PR is a completely redone paragraph or so about the needed Java 8 and Node versions. I made sure to link to nvm and Jabba. Please let me know if this doesn't read well.

Mention of Docker

I'm on the fence. I suppose that this will be a project for rainy day!

Publish

I can just do it for now - no problem.

Thanks!

MatthewVita commented 3 years ago

(also I branched off of the https://github.com/MatthewVita/node-hl7-complete/pull/18 branch for this just to make this simple)

privateOmega commented 3 years ago

@MatthewVita Could we please release a version 3.0.3, merge #18 and then release 3.1.0 from #20 ?

Reasoning behind it is even people using node < 12 can still use this package with and could get improved hl7 parsing support with the latest version of hapi which was merged by me earlier. What do you say?