Node 18

[MikiDi]

ℹ️ A built image of 6f49345 is available at semtech/mu-javascript-template:feature-node-18

[MikiDi]

⚠️ 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]

@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]

#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]

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]

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

[nvdk]

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 af211ed . I don't think this is required anymore

[nvdk]

@erikap could we get this merged?

[madnificent]

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]

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]

Posting here since I can't seem to open the discussion here: #51 (comment)

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]

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 #51 (comment)

[madnificent]

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:

• it fails since node 16 and works in node 15 (tested 15, 16, 18, and 20)
• it fails both on Chromium as well on Brave
• it does work when we bind in docker-compose using 127.0.0.1:9229:9229
• it does work when we bind in docker-compose using 0.0.0.0:9229:9229
• it works when explicitly adding 127.0.0.1:9229 in inspector targets in the browser (only tested on Chromium)
• it works on native node 20 (and hence we expect on the others too)
• it also fails when adding ipv6 support to the Docker daemon and giving the containers an ipv6 address
• setting the --dns-result-order=ipv4first nodecli option does not resolve the issue
• it appears on a system in which /etc/hosts has ::1 localhost set as well as where it is not set

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]

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]

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]

Seems like everything is working. Decorators are untested.

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