Unexpected token error when running rosbridge with rclnodejs 0.18.0

meyerj commented 3 years ago

Since the upgrade of rclnodejs from 0.17.0 to 0.18.0 running the rosbridge (ros2-web-bridge) fails with

# node /node_modules/ros2-web-bridge/bin/rosbridge.js
/external/rclnodejs/lib/context.js:47
  static _instances = [];
                    ^

SyntaxError: Unexpected token =
    at Module._compile (internal/modules/cjs/loader.js:723:23)
    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 Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.<anonymous> (/external/rclnodejs/index.js:20:17)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)

The environment is pretty standard, I guess:

docker image ros:foxy-ros-base
- ROS 2 Foxy
- Ubuntu 20.04.1 Focal

$ git submodule status
+b082bb74a75c1773d70b2e00850468779374cd00 external/rclnodejs (0.18.0)
+248f2ee44c525ebbd7c90e7c056ba62f3939eb02 external/ros2-web-bridge (0.3.0)

```
# node --version
v10.23.1
```

We build rclnodejs and ros2-web-bridge from source in a Dockerfile with the commands

# install Nodejs
RUN curl -sL https://deb.nodesource.com/setup_10.x | bash - && apt-get -y install nodejs
COPY external/ros2-web-bridge /external/ros2-web-bridge
COPY external/rclnodejs /external/rclnodejs
RUN /bin/bash -c ". /opt/ros/$ROS_DISTRO/setup.bash && npm i /external/rclnodejs && npm i /external/ros2-web-bridge"

Any idea what could be wrong? When checking out version 0.17.0 in submodule rclnodejs, it works (but with the issue in #752).

meyerj commented 3 years ago

RUN curl -sL https://deb.nodesource.com/setup_10.x | bash - && apt-get -y install nodejs

Upgrading to Node.js 12.x (v12.20.1) solves the issue. In that case the Prerequisites section in the README.md may need an update?

minggangw commented 3 years ago

I deem the description of the required Node.js is outdated, it was tested a long time ago, so it needs to be corrected.

minggangw commented 3 years ago

I look through the current codebase and found we used some ES2019 features that node < v10.x doesn't support, so I think that caused the problem you met.

Usage of static class fields that gets supported since Node.js 12.4.0 https://github.com/RobotWebTools/rclnodejs/blob/31efa7c28e8d20bcd6fe1311fb10e9afa7bd63a7/lib/context.js#L47
Usage of Array.prototype.flat that gets supported since Node.js 11.15.0 https://github.com/RobotWebTools/rclnodejs/blob/31efa7c28e8d20bcd6fe1311fb10e9afa7bd63a7/rosidl_gen/packages.js#L138-L140

Also, you can check it on https://node.green to see the detailed status. So I think it's the time to update the README accordingly, thanks for your feedback!

formigarafa commented 3 years ago

will support for anything less than 12 be dropped or the code will change to keep compatibility?

wayneparrott commented 3 years ago

@minggang I recognize that some of my contribs helped create this node compatibility problem. Propose the following:

we consider including a polyfill library such as core-js to enable a more modern coding experience without introducing compat issue such as those you highlighted.
update the contribution guidelines to explicitly call out the js spec level requirements for devs to adhere to.
add compatibility validation to our test/CI. Thoughts?

minggangw commented 3 years ago

I have submitted a PR to add the verifications of Node.js 8.x and 10.x, the results are

minggangw commented 3 years ago

1. we consider including a polyfill library such as [core-js](https://www.npmjs.com/package/core-js) to enable a more modern coding experience without introducing compat issue such as those you highlighted.

I prefer we could change the cases that use the latest features to the compatible patterns because it seems that these cases are limited.

2. update the contribution guidelines to explicitly call out the js spec level requirements for devs to adhere to.

+1 and the CI could check it too.

3. add compatibility validation to our test/CI.

Have submitted #773

RobotWebTools / rclnodejs

Unexpected token error when running rosbridge with rclnodejs 0.18.0 #768