Support running Insect with Node 10

[triallax]

TODO:

• [x] Write a detailed description and commit message
• [ ] Test different Node versions in CI? (given that this PR only makes Insect run, not build, with Node 10, and that Spago only supports Node 12+, this may or may not prove somewhat troublesome)

[sharkdp]

Thank you. I managed to run Insect on Ubuntu 20.04 using this branch. It looks like I still need the node_modules folder. index.min.cjs alone is not enough.

---

Users with node versions < 12 can use the following Dockerfile to build Insect (it can also be used to run Insect with node 18). This can almost certainly be improved.

FROM node:18

WORKDIR /usr/src/insect

COPY . .

RUN npm install -g purescript --unsafe-perm && \
    npm install -g spago && \
    npm install && \
    npm run build

ENV XDG_DATA_HOME /tmp

CMD ["node", "index.min.cjs"]

After creating the image (docker build -t sharkdp/insect .), they can create the container and copy out the build artifacts:

docker create sharkdp/insect:latest
# copy SHA (e.g. 71f0797703e8)
docker cp 71f0797703e8:/usr/src/insect/index.min.cjs .
docker cp -r 71f0797703e8:/usr/src/insect/node_modules .

This workflow can also almost certainly be improved :smile:

To directly run insect inside the Docker (paying a heavy startup time penalty of ~700ms), you can use

docker run -it --rm sharkdp/insect:latest

[triallax]

Thank you. I managed to run Insect on Ubuntu 20.04 using this branch. It looks like I still need the node_modules folder. index.min.cjs alone is not enough.

Correct, that is intentional, as npm install insect will install Insect's dependencies into node_modules anyway, so there's no reason to bundle them.

---

I don't use Docker, but your work looks neat. Maybe we can include it somewhere in the documentation?

[triallax]

By the way, I think the manual installation of PureScript and Spago in the Dockerfile isn't necessary, is it? Both are in devDependencies in package.json.

[sharkdp]

Correct, that is intentional, as npm install insect will install Insect's dependencies into node_modules anyway, so there's no reason to bundle them.

Ah. I saw a lot of warnings concerning esbuild and assumed that the npm install step had failed under node 10, but it seems to work fine :+1:

By the way, I think the manual installation of PureScript and Spago in the Dockerfile isn't necessary, is it? Both are in devDependencies in package.json.

Correct. I played around in the docker container interactively before and somehow needed that. Confirming that the following also works:

FROM node:18

WORKDIR /usr/src/insect

COPY . .

RUN npm install && \
    npm run build

CMD ["node", "index.min.cjs"]

I also removed the ENV command. Users then have to mount in their local insect-history file like this:

docker run -it --rm -v "$HOME"/.local/share/insect-history:/root/.local/share/insect-history sharkdp/insect:latest

Does it make sense to share that Dockerfile in the repo? Or maybe even upload the docker image to Docker hub?

[sharkdp]

Does this PR have any influence on the startup time? This is what I'm getting on node 10

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
Startup time (insect '1+1')	301.9 ± 1.4	299.2	304.0	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
Startup time, stdin mode (insect < computations-1.ins)	306.5 ± 3.2	298.5	310.0	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
Evaluation speed (insect < computations-160.ins)	779.4 ± 21.2	761.8	837.6	1.00

Seems to be quite a bit worse than what we had here https://github.com/sharkdp/insect/pull/322, but obviously that's two different machines (and node versions!).

[triallax]

Does this PR have any influence on the startup time?

Ah, I forgot about that, thanks for reminding me about that.

Seems to be quite a bit worse than what we had here https://github.com/sharkdp/insect/pull/322, but obviously that's two different machines (and node versions!).

That's because the benchmarks run index.js, which with this PR's changes imports unbundled JavaScript code spread across numerous files (basically the situation pre-#322). However, users of Insect would actually be running the bundled and minified index.min.cjs. I just modified the benchmark script to run index.min.cjs instead of index.js, which makes much more sense.

Using the modified benchmark script with this PR, here are the results on my machine (with Node 18.7.0):

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
Startup time (insect '1+1')	161.4 ± 1.8	157.7	164.5	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
Startup time, stdin mode (insect < computations-1.ins)	180.5 ± 7.5	176.3	206.1	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
Evaluation speed (insect < computations-160.ins)	1047.8 ± 15.0	1028.2	1072.1	1.00

On master:

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
Startup time (insect '1+1')	179.9 ± 1.3	178.2	183.4	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
Startup time, stdin mode (insect < computations-1.ins)	199.9 ± 2.0	196.9	202.5	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
Evaluation speed (insect < computations-160.ins)	1079.1 ± 25.9	1044.4	1124.0	1.00

This PR is a little faster, which makes sense, given that with master, index.js is run, which then imports insect.js, whereas with this PR the contents of insect.js are bundled into insect.min.cjs, so we get rid of that indirection.

[triallax]

What are your thoughts about testing the various supported Node.js versions in CI? If you like the idea, how do you think we should go about it?

[sharkdp]

I think we shouldn't put too much burden on us. Sure, a matrix build sounds great, but I'd be completely fine with testing the lowest supported version for now. Or do we expect the PureScript compiler to output code that would break in newer versions of node?

[triallax]

The issue here isn't testing multiple Node versions, the issue is testing with Node 10, as it is unable to build Insect, only run it.

Or do we expect the PureScript compiler to output code that would break in newer versions of node?

I don't think we can expect the compiler to, no, but I do believe that our build system might cause breakage.

[sharkdp]

The issue here isn't testing multiple Node versions, the issue is testing with Node 10, as it is unable to build Insect, only run it.

Oh, right. Of course. So for that case, we should probably install both Node 10 and a newer version within a single container?

Or do we expect the PureScript compiler to output code that would break in newer versions of node?

I don't think we can expect the compiler to, no, but I do believe that our build system might cause breakage.

Ok.

[triallax]

So for that case, we should probably install both Node 10 and a newer version within a single container?

Any ideas how we can do that?

[sharkdp]

To be honest, I don't know. Maybe setup-node can install multiple versions at once? Maybe it can be called twice in a row? Or maybe we call it once, build everything, call it a second time (for node 10) and then run the tests?

[triallax]

Frankly, I don't feel like working on Node 10 testing in the CI (at least for the time being). I still think it's a good idea though, so anybody can feel free to work on it. For now, I'm marking this PR as ready for review; if it looks good to you, please feel free to merge it (please also check out my review comments above).

[triallax]

Does it make sense to share that Dockerfile in the repo?

I think that's a good idea. Where do you suggest we put it and document it?

[triallax]

This should be ready. Please take a last look at my changes and then merge if everything looks good.

[sharkdp]

Thank you very much!

Final question: does this change anything with the release process to NPM (npm publish and everything that is necessary before)? I really should have written down some documentation for how to publish to NPM, because I can never remember what to do exactly. And what not to do. Just running npm publish from a clean repo is not good because we want to include the compiled JS output in the published version (right?).

[triallax]

Final question: does this change anything with the release process to NPM (npm publish and everything that is necessary before)?

To be honest, I do not know what the previous release process for NPM is, so I cannot comment on that part. However, I believe that with this PR, you don't need to do anything more than npm install (or is it npm ci?) before npm publish.

I really should have written down some documentation for how to publish to NPM, because I can never remember what to do exactly. And what not to do. Just running npm publish from a clean repo is not good because we want to include the compiled JS output in the published version (right?).

npm prepack should be automatically run before npm publish packs the NPM tarball, and it does build the index.cjs that should be included in the published version. So, as stated above, I think from a clean repo, npm ci && npm publish is really all you need.

Of course, it would probably be a good idea to test this first, perhaps by making a clean clone then running npm ci && npm publish --dry-run.

[sharkdp]

Thank you. The test worked great.

[triallax]

I just learned that there is also another command that I forgot to mention: ./docs/generate (to generate/update the manpage). This means that the full command to publish should be ./docs/generate && npm ci && npm publish.

I considered running ./docs/generate in the prepack hook, but that would not work for Windows users not using Cygwin/WSL/whatever, since it is a POSIX shell script. As an alternative, I think we could either:

1. Add a script to publish the NPM package (my preference)
2. Write the publishing process down somewhere

Thoughts?

[sharkdp]

I think we should do both :smile:. And the documentation should simply be sth like this

To publish a new version of Insect:

• Bump the version in package.json
• Run XXX to also update the version in the lock file
• Run the publish-new-version.sh script.

[triallax]

Hmm, sounds good, I'll see if I can open a PR for this.