Re-implement simple dependencies internally #8
Conversation
This comment has been minimized.
This comment has been minimized.
rmg
force-pushed
the
in-tree-deps
branch
2 times, most recently
from
71af900
to
2e83587
Compare
|
I think its unfortunate that we have to inline such a common stream utility. byline isn't leftpad, it took 86 new lines for that change alone here (and yes, I count test code, we didn't need the test code when using byline, which already had tests). I'd rather see byline adopted by someone who can maintain it, like us. But, in the meantime, we have a problem, and we have to fix it, so LGTM. |
|
We could fork byline :-(. I brought this up in nodejs/package-maintenance#4 (comment), FYI |
In this case I don't count the test lines. We happened to inherit the behaviour from byline, but if byline's behaviour changed we wouldn't know it without those tests. I'm back-porting them in #9 because this PR has turned in to a major rewrite/refactor and as such shouldn't be modifying any tests. |
Rather than continue depending on an external module with its own dependencies, not all of which are still maintained, replace our usage of it with a minimal implementation of a line splitting Transform stream. In addition to the StringDecoder related tests, add a blank line to one of the test examples so that we can make sure we enforce the current behaviour of swallowing empty lines. This implementation follows the same general patter as byliner, which is to use a StringDecoder to accumulate bytes so we can ensure that we're not assuming everything is single byte characters and accidentally interpreting a random byte in the middle of a multi-byte sequence as something it isn't and then corrupting things as a result. For anyone looking to level up their use of streams, this is the kind of thing most people don't think of and an approach you should absolutely consider when dealing with messages on streams that may be split across reads such as splitting a multi-byte UTF8 character across a packet boundary on a TCP request. Signed-off-by: Ryan Graham <[email protected]>
The code for this is small and it decreases our dependency on 3rd party modules that may no longer be maintained. Testing is provided by our existing test suite, including 100% coverage of this duplex stream implementation. Signed-off-by: Ryan Graham <[email protected]>
Signed-off-by: Ryan Graham <[email protected]>
The convenience API provided by the through module is no longer worth the cost of the dependency because the API provided by streams in core is equally convenient for our uses. Signed-off-by: Ryan Graham <[email protected]>
tap 12.0.1 was a security release, but depended on nyc@13 which doesn't support node 4 (tapjs/tapjs#479). Fortunately, v12.1.1 has been released to fix that. Bumping the version here to bust caches that may still think 12.1.0 is the best match. Signed-off-by: Ryan Graham <[email protected]>
|
The replacement of I'm closing this PR now while I work on other things. The changes will probably come back in the form of a semver-major release that also drops the CLI which will allow all dependencies to be removed. |
While re-implementing the line splitting functionality of byline to resolve #7, it became obvious that the other convenience dependencies weren't really pulling their weight.