Benefits of `callHookFnSync()` and `callHookFnAsync()`:
* They are a lot more forgiving than `hookCallWrapper()` was.
* They perform useful sanity checks.
* They have extensive unit test coverage.
* They make the behavior of `callFirst()` and `aCallFirst()` match
the behavior of `callAll()` and `aCallAll()`.
Define states and use them to properly handle multiple calls to
`start()`, `stop()`, and `exit()`. (Multiple calls to `exit()` can
happen if there is an uncaught exception or signal during shutdown.)
This should also make it easier to add support for cleanly restarting
the server after a shutdown (for tests or via an `/admin` page).
* lint: skin-variants
* for squash: Fix attachment of event listener
Before this PR the statement was outside the function. I'm assuming
the move into the function body was accidental, so move it back out.
* for squash: Preserve order of function calls
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
There are some problems with nyc:
* The coverage numbers aren't useful in our case because most of the
code is executed outside the test process (the test code is mostly
API client logic).
* nyc messes with line numbers, which makes it much harder to debug
problems.
* We're seeing frequent SIGABRT crashes while nyc is printing the
results table. I'm not sure if nyc is the cause of the crashes, or
if it's making a race condition worse, or if the crashes have
nothing to do with nyc, but we don't lose much by removing it so
we might as well see if the crash frequency improves.
Before this change, the `author` attribute was silently discarded
during `.map()` iteration and the name of the attribute to remove was
included twice with two different values.
Before this commit, the callback passed to `.map()` during attribute
removal was a normal function, not an arrow function. This meant that
the value of `this` in the function body depended on how the callback
was invoked. In this case, the callback was invoked without any
explicit context (it was not called as a method, nor was it called via
`.call()`, `.apply()`, or `.bind()`). Without any explicit context,
the value of `this` depends on strict mode. Currently the function is
in sloppy mode, so `this` refers to the "global this" object (a.k.a.,
`window`). It doesn't make sense for the callback to reference
`window.author`, so I'm assuming the previous behavior was a bug.
Now the function is an arrow function, so the value of `this` comes
from the enclosing lexical context, which in this case is the
AttributeManager object. I believe that was the original intention.
This makes it possible for plugin backend tests to do
`require('ep_etherpad-lite/tests/backend/common')` to access the API
key (among other things).
Eventually we probably should reverse these (move `tests/` to
`src/tests/` and make `tests/` a symlink to `src/tests/`) and move
`bin/` to `src/bin/` so that we can avoid the top-level `package.json`
mess.
The `name` property is only available on cheerio's Element-like
objects; DOM Element objects do not have a `name` property. Switch to
`dom.tagName()` to fix the logic for browsers.
The `parent` property is only available on cheerio's Node-like
objects; DOM Node objects do not have a `parent` property. Switch to
the `parentNode` property so that the code works in browsers as well
as cheerio.
Before, the hook always ignored the return values provided by the hook
functions. Now the hook functions can change the text by either
returning a string or setting `context.text` to the desired value.
Also drop the `styl` and `cls` context properties. They were never
documented and they were always null.
Richard Hansen