search

mu-semtech / mu-javascript-template

Template for running javascript/express microservices
4 stars 17 forks source link

Node 18 #51

Closed MikiDi closed 1 year ago

MikiDi commented 1 year ago

:information_source: A built image of https://github.com/mu-semtech/mu-javascript-template/pull/51/commits/6f493450c89dbfd69bb33e0f13cb0457013df3f3 is available at semtech/mu-javascript-template:feature-node-18

MikiDi commented 1 year ago

:warning: To use the Chrome DevTools debugger in the Brave browser (version 1.48.158 Chromium: 110.0.5481.77 (Official Build) (64-bit)) with this image, it is required you bind the debugger port as as 127.0.0.1:9229:9229 instead of just as 9229:9229. Failing this, the service won't show up as as "Remote Target" in the debugger. cc @erikap

nvdk commented 1 year ago

@MikiDi any specific reason this is marked as draft? are there any breaking changes (aside from the node bump) that we should be aware of?

MikiDi commented 1 year ago

https://github.com/mu-semtech/mu-javascript-template/pull/46 lead me to believe that merging this without work on the transpiling script, TS support would be broken. Some feedback on that would be appreciated. Apart from that, testing didn't bring up anything specific except the debugger issue above.

MikiDi commented 1 year ago

I also was unsure about the removal of the unsafe perm option. I removed that to fix the deprecation, but everything still seems to work, which makes me wonder why it was there in the first place.

nvdk commented 1 year ago

very basic testing on https://github.com/redpencilio/ldes-consumer-service/ seems to indicate that builds just fine at least.

nvdk commented 1 year ago

I also was unsure about the removal of the unsafe perm option. I removed that to fix the deprecation, but everything still seems to work, which makes me wonder why it was there in the first place.

did some sleuthing and it seems this was introduced because npm prepare scripts had issues otherwise, the workaround is referenced in commit https://github.com/mu-semtech/mu-javascript-template/commit/af211edc77791cd0bef0dc3d8966b1e87c3b26d9 . I don't think this is required anymore

nvdk commented 1 year ago

@erikap could we get this merged?

madnificent commented 1 year ago

Looking at this with @erikap. We do not see testing or confirmation that TypeScript and CoffeeScript are still supported with this PR in chat messages (but we may miss some) nor in the comments on the PR. Needs more study from our end to figure out if this works with the features the template supports today :/

The total amount of changes in the PR are not huge (yay!) but we should verify nonetheless.

madnificent commented 1 year ago

We can confirm TypeScript and CoffeeScript still work for app.ts and app.coffee respectively.

The issue with Brave requiring an explicit bind as 127.0.0.1:9229:9229 is still a mystery.

Windvis commented 1 year ago

Posting here since I can't seem to open the discussion here: https://github.com/mu-semtech/mu-javascript-template/pull/51#discussion_r1330208343

Some tweaks would be needed to enable the babel plugins again that are required for the classic decorators but I wonder if it would make sense to instead remove classic decorator support? micro services might want to start using stage 3 decorators now? Do we use decorators in microservices somewhere?

nvdk commented 1 year ago

Looking at this with @erikap. We do not see testing or confirmation that TypeScript and CoffeeScript are still supported with this PR in chat messages (but we may miss some) nor in the comments on the PR. Needs more study from our end to figure out if this works with the features the template supports today :/

The total amount of changes in the PR are not huge (yay!) but we should verify nonetheless.

for TS support, yes it works see https://github.com/mu-semtech/mu-javascript-template/pull/51#issuecomment-1629084450

madnificent commented 1 year ago

Only smoke tests so far, I read.

Regarding decorators, yes we have code that uses decorators but it's quite limited to my knowledge and it could be upgraded. We should check what the upgrade path is and if that is feasible.

With respect to the port binding we notice that it seems broken since node 16 but we cannot pinpoint the issue. There's some weight to it because it seemingly breaks the abstraction that you can bind a port and have something that works, and breaking such abstractions is especially costly to new developers because they'll start stabbing in the dark. But also for our own understanding, as we don't really know why it's acting like this ourselves. Input very welcome.

We notice the following when trying to find where it goes wrong:

Our research pointed to an IPv6 issue but that might not be the issue at all (the change we found is at node 17 rather than node 16).

@MikiDi could you confirm that your node inspector picks this up, and if it does, could you check what targets are set?

nvdk commented 1 year ago

since the bind was already broken, could we move that to a separate issue? don't think it should block merging this PR. imo it's also easily remedied with an addition in the README

madnificent commented 1 year ago

TL;DR of current state: debugger works again. Checking comments to see if we missed something else.

Switch from alpine to Debian bookworm for node inspector

Shifting to debian means we need to explicitly use bash.

When using the node debugger through chromium based browsers (such as Brave) the node debugger fails to connect to the nodejs inspector. Together with @erikap we inspected the packets and it seems nodejs does not accept incoming connections on the ipv6 address as its hostname. It is unclear why it does so. It does accept connections on the specific ipv4 address 127.0.0.1. Searching for solutions it seems Debian Bookworm uses different basic settings (though it is unclear which those are exactly) that do make this case work.

In case of alpine there seems to be an ipv6 incompatibility. Explicitly adding 127.0.0.1:9229 as an inspect target makes things work. Explicitly binding an ipv4 port in Docker (making it drop the IPv6 port) makes things work too. Using an older NodeJS version (up to alpine node:15) also makes it work. We therefore conclude this is a compatibility issue and decide not to dig further.

We have dug into the packages. We have searched online. People seem to accept this is the state of NodeJS and lacking deep dive guides regarding nodejs, this seems to be the most straight path forward.

If anyone wants to dig, feel free to share what you learn.

madnificent commented 1 year ago

Seems like everything is working. Decorators are untested.

Thanks a lot for all your attentive input to land node-18 support :tada: