For some reason strings are sometimes passed to `findUnmet()`, which
is obviously unexpected given the way the code is written. Rather than
figure out why strings are passed and how to safely avoid passing
strings, just return early. The net effect is the same, but returning
early avoids setting a property on a string, which is prohibited in
strict mode.
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.
In the DOM, `.children` only includes children that are Element
objects. In cheerio 0.22.0, `.children` includes all child Nodes, not
just Elements. Use `dom.numChildNodes()` and `dom.childNode()` so that
browsers behave the same as cheerio.
`for..in` iterates over inherited properties, which is almost never
desired. In most cases there aren't any inherited enumerable
properties so it's not that big of a deal, but in the case of
HTMLCollection it's very bad because it iterates over every entry
twice (once by numerical index and once by name) plus it includes the
`length` property in the iteration.
The `attribs` property is only available on cheerio's Element-like
objects; DOM Element objects do not have an `attribs` property. Switch
to `dom.nodeAttr()` to fix the logic for browsers.
Various tidy up and linting of contentcollector.js and domline.js.
3 Tests disabled which are not due to be covered.
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
* remove IE and add strict headers
* linting: kids are back, need to stop for today
* linting: farbtastic fix
* lint: more lint fixes
* more lint fixes
* linting: sub 100 errors
* comments where I need help
* ready to be helped :)
* small fixes
* fixes
* linting: all errors resolved
* linting: remove note to self
* fix as per nulli/wezz000li suggestion
* fix as per nulli/wezz000li suggestion
* resolve merge conflicts
* better use if to silence eslint
* Use `for..of` with `Object.keys` instead of `for..in`
* lint: move setSelection to before call
Co-authored-by: webzwo0i <webzwo0i@c3d2.de>
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
This will make the pages gracefully handle HTTP server restart events,
which happen whenever a plugin is installed or uninstalled via the
`/admin/plugins` page.
* lint: collab-client
* Undo incorrect lint fixes
These will be re-fixed in a future commit.
* Properly fix guard-for-in error
* Properly fix prefer-rest-params errors
* Move some code back to where it was
Moving the code makes it hard to review the diff.
* Delete DISCONNECT_REASON case
Someone reading the code won't understand what "used to handle
appLevelDisconnectReason" means until they dig through the Git
history. Given the server never sends messages of type
DISCONNECT_REASON anyway, just delete the case.
* Refine lint fixes
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
Squashed changes from rhansen@rhansen.org:
* Move code back to where it was. (It's easier to review changes
when the code isn't moved. This causes some no-use-before-define
warnings to reappear, but those are just warnings.)
* Move eslint-disable comment to same line
* Use `window.clientvars` to resolve no-global-assign
* Undo changes that aren't about fixing lint errors
* lint: pluginfw tsort.js
* Don't comment out the `console.log()` call
Disabling the log message is out of scope for the pull request.
* Put const and let on separate lines
* Convert `tsort` from function to arrow function
ESLint doesn't complain about this due to a bug in
prefer-arrow/prefer-arrow-functions rule:
https://github.com/TristonJ/eslint-plugin-prefer-arrow/issues/24
Co-authored-by: Richard Hansen <rhansen@rhansen.org>
* fix accidental write to global variable
properly show pending tests
log test name in suite
better log output for received/expected strings
* cc tests: enable second nestedOL test
* ignore the head tag on import
Richard Hansen