Repoint node-memwatch to fork which supports node 18 #47
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asdf [1] is a tool for running multiple versions of different languages. To use it one installs asdf itself, and then 'plugins'. For this case, we will need the nodejs plugin [2]. Then, to use asdf, one runs: ``` asdf install nodejs 16.15.0 asdf reshim nodejs ``` The reshim is important, otherwise asdf won't pick up the latest versions that have been installed for a given plugin. To set a default version for a project one uses a .tool-versions file. In this commit this file has been set to default nodejs to 16.15.0. [1]: https://asdf-vm.com/ [2]: https://github.com/asdf-vm/asdf-nodejs
@aide-master/node-memwatch hasn't been updated for 3 years [1], so it might be worthwhile exploring other forks like @airbnb/node-memwatch that have been more recently touched. After blowing away package-lock.json and rebuilding it with node 16.15.0, it appears that the node-gyp issue per [2] is fixed, so this seems sufficient. [1]: https://www.npmjs.com/package/@aidemaster/node-memwatch [2]: andywer#34
Upon running the tests locally I noticed that after altering the packages things failed. However CI on github is currently green. To mitigate against this form of change management risk adding a github action to exercise the test harness seems wise.
token-cjg
force-pushed
the
bump-to-node16
branch
from
5d30614 to
2d68400
Compare
Since we've switched from @aidemaster/node-memwatch to @airbnb/node-memwatch, we should update the source to reflect this.
Allows for step through debugging of tests
It appears that there is a bug in the 16.x.x series [1] that amongst
other things leads to memory leaks when attaching a memory watch process
to a noop process (i.e., () => {}).
To work around this, bump to the latest node. 18.x.x will soon be
active anyway [2], so this seems okay to do.
[1]: nodejs/node#28787
[2]: https://nodejs.org/en/about/releases/
token-cjg
force-pushed
the
bump-to-node16
branch
from
cde08af to
717001b
Compare
token-cjg
changed the title
A previous test run failed with "AssertionError: expected 25864 to be below 24576" [1] This seems much of a muchness. To avoid flakes let's just increase the margin that we're testing against a tad. [1]: https://github.com/token-cjg/leakage/runs/8145013267?check_suite_focus=true
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change repoints the node-memwatch package dependency to a different fork (@airbnb/node-memwatch) which supports node 18.
Also in this change is a slight implicit change in development tooling to suggest the use of asdf (https://asdf-vm.com/), as well as a quick sweep of security vulnerabilities using
npm audit fix --force.In particular this proposed alteration should address #34.