Many changes #166

dougg0k · 2022-09-06T00:34:13Z

Hello!

A few days ago I looked at the project and got interested in it, seemed to be a better option than bee-queue. And I looked through the issues and made this PR.

But this PR contains more things than that.

It seem to address this, at least the lua are sent to dist. Broken lua filepaths when transpiled/bundled #114
I completely addressed this. dependencies cleanup #68
I migrated the project from npm to pnpm which are very fast. and is the one I have been using in my projects. After trying to install packages using recent npm version, it that was complaining / giving errors.
I upgraded most packages, with some exceptions that has breaking changes, but they were partially upgraded, to the point that compatibility was still there.
I fixed type errors with and without additional checks.
Imports were re-ordered alphabetically.

It could be worth upgrading to ioredis latest version, seems that is more stable but there are breaking changes. As for the p- starting dependencies, it is in the latest version that is usable in non-ESM projects. While the rest are up-to-date.

Edit: I upgraded ioredis to v5, only small breaking changes, mostly in the typing.

Edit2: There is a test that randomly fails. Been happening before I started the changes though. Sometimes I get consecutives test run all successful and sometimes consecutives 1 fail with that one below.

createOrchestrator › createOrchestrator marks orphaned task as processing
> 78 |     expect(await isTaskStalled({ taskId, queue, client })).toBe(true);

It's up to you to accept or not. All tests seem to be passing.

@jasrusable

jasrusable · 2022-09-07T08:11:42Z

Thanks for this PR! I'll take a look through it when I have some time.

dougg0k · 2022-10-06T15:23:55Z

Hey @jasrusable are you able to get this merged in?

dougg0k · 2022-10-13T16:14:20Z

@jasrusable ? I wanted to use the project with these changes.

jasrusable

Hey @dougg0k :) I took a quick look through the PR, for the most part I think it looks great! Thanks again.

Two main things though:

I'd prefer to stick with vanilla npm for the sake of its ubiquity for now.
I'd prefer to use date-fns over datejs due to its simplicity.

Would you mind please implementing those changes?

jasrusable · 2022-10-13T19:33:17Z

Edit2: There is a test that randomly fails. Been happening before I started the changes though. Sometimes I get consecutives test run all successful and sometimes consecutives 1 fail with that one below.

I'll take a look at this test and see if I can spot why it might be flaky.

dougg0k · 2022-10-13T20:17:03Z

Hey @dougg0k :) I took a quick look through the PR, for the most part I think it looks great! Thanks again.

Two main things though:

I'd prefer to stick with vanilla npm for the sake of its ubiquity for now.

I'd prefer to use date-fns over datejs due to its simplicity.

Would you mind please implementing those changes?

npm gives problems on install, I would need to use -legacy-peer-deps flag to make it work.
dayjs are much smaller and has the same features/api as moment, while being immutable. dependencies cleanup #68 (comment)

@jasrusable

jasrusable · 2022-10-14T07:20:03Z

What errors does npm give on installation? Can you share the error logs?

dougg0k · 2022-10-14T11:47:42Z

Nevermind, it did while I was working on it.

dougg0k · 2022-10-14T12:02:49Z

https://github.com/raineorshine/npm-check-updates Much better and easier to update libs while not having the bloat of dependabot and attempts of lib updates that goes to ESM only. You can just --reject those.

jasrusable · 2022-10-14T12:51:36Z

Great, then last few things:

Switch datejs to date-fns
Fix failing tests

dougg0k · 2022-10-14T13:08:28Z

May I ask why keep it compatible with older versions than 14 LTS?

Because of older versions I had to downgrade Jest, which required to downgrade Typescript and now other libraries and now the need for --legacy-peer-deps appeared.

It's prefered to keep oudated libs for the sake of old nodejs versions? @jasrusable

falkenhawk · 2022-10-14T13:25:28Z

package.json

+    "dayjs": "1.11.5",
+    "debug": "4.3.4",
+    "ioredis": "5.2.3",
+    "p-queue": "6.6.2",
+    "p-timeout": "4.1.0",
+    "rate-limiter-flexible": "2.3.12",
+    "set-interval-async": "3.0.2",
+    "uuid": "9.0.0"


I don't think dependencies in "library"-like packages should be fixed to exact versions. Could we still have carets here, please?

Isn't that the whole point of have proper versions and to keep compatibility? Caret may install a different version than what you see.

caret are minor + patch. Maybe no breaking changes if every lib are following the semantics.

dougg0k · 2022-10-22T17:39:00Z

Can you re-run the CI? @jasrusable

dougg0k · 2022-10-22T21:08:41Z

Now it's one incompability with set-interval-async in nodejs v12 and that test I said before that was failing randomly, so much they have all succeeded in most runs but not in one.

Can I drop v12 from tests, too?

Will you take a look at that failing test? @jasrusable
It seems there were 2, but here I only ever seen 1 failing randomly.

jasrusable · 2022-10-22T21:16:53Z

Now it's one incompability with set-interval-async in nodejs v12 and that test I said before that was failing randomly, so much they have all succeeded in most runs but not in one.

Can I drop v12 from tests, too?

Will you take a look at that failing test? @jasrusable It seems there were 2, but here I only ever seen 1 failing randomly.

I’ll take a look at the remaining failing tests.

dougg0k · 2022-10-22T21:18:01Z

Cool, thanks.

jasrusable · 2022-10-25T08:30:14Z

@dougg0k seems it's just a single test that fails intermittently. Can you try increasing the sleep time to see if that fixes the test?

dougg0k · 2022-10-25T12:39:33Z

Yeah, that is the test I mentioned in the PR first msg, I already tried increasing before, didnt work. @jasrusable

dougg0k · 2022-10-25T23:57:08Z

I mean, if passes and fails randomly with the same value, must be a bug somewhere.

After trying multiple runs. I saw some random fails of.

 FAIL  src/tests/actions/acknowledge-task.test.ts
  ● acknowledgeTask › acknowledgeTask acknowledges task

    expect(received).toBe(expected) // Object.is equality

    Expected: true
    Received: false

      37 |     expect(await isTaskStalled({ taskId: task.id, queue, client })).toBe(false);
      38 |     await sleep(50);
    > 39 |     expect(await isTaskStalled({ taskId: task.id, queue, client })).toBe(true);

It seems to be related to the same bug. Where it expect the task to be stalled but are not.

@jasrusable

dougg0k · 2022-10-26T23:01:01Z

I tried running where one time succeeded and one failed. Any idea what could cause it to behave like this? @jasrusable
areTasksStalled

Seems like taskAcknowledgeKeys key does not yet exist when failed.

Succeeded	Failed
console.log results - [] `at src/actions/are-tasks-stalled.ts:30:11` console.log stallingTasksIds - [] `at src/actions/are-tasks-stalled.ts:31:11` console.log results - [1] `at src/actions/are-tasks-stalled.ts:30:11` console.log stallingTasksIds - ["1bb5345d-46b9-4f0a-831a-38cd46bb871e"] `at src/actions/are-tasks-stalled.ts:31:11` console.log results - [1] `at src/actions/are-tasks-stalled.ts:30:11` console.log stallingTasksIds - ["1bb5345d-46b9-4f0a-831a-38cd46bb871e"] `at src/actions/are-tasks-stalled.ts:31:11` console.log results - [0] `at src/actions/are-tasks-stalled.ts:30:11` console.log stallingTasksIds - ["1bb5345d-46b9-4f0a-831a-38cd46bb871e"] `at src/actions/are-tasks-stalled.ts:31:11` console.log results - [0] `at src/actions/are-tasks-stalled.ts:30:11` console.log stallingTasksIds - ["1bb5345d-46b9-4f0a-831a-38cd46bb871e"] `at src/actions/are-tasks-stalled.ts:31:11</td>`	console.log results - [] `at src/actions/are-tasks-stalled.ts:30:11` console.log stallingTasksIds - [] `at src/actions/are-tasks-stalled.ts:31:11` console.log results - [1] `at src/actions/are-tasks-stalled.ts:30:11` console.log stallingTasksIds - ["56733eb8-1857-42e7-9b32-6a0dcd13b1d1"] `at src/actions/are-tasks-stalled.ts:31:11` console.log results - [0] `at src/actions/are-tasks-stalled.ts:30:11` console.log stallingTasksIds - ["56733eb8-1857-42e7-9b32-6a0dcd13b1d1"] `at src/actions/are-tasks-stalled.ts:31:11` console.log results - [] `at src/actions/are-tasks-stalled.ts:30:11` console.log stallingTasksIds - [] `at src/actions/are-tasks-stalled.ts:31:11` console.log results - [] `at src/actions/are-tasks-stalled.ts:30:11` console.log stallingTasksIds - [] `at src/actions/are-tasks-stalled.ts:31:11`

I will leave the rest to you.

jasrusable · 2022-10-27T09:21:19Z

Strange, I cannot reproduce this failing test locally at all now, with either combination of Node 14, 16 or 18, Redis 4 or 5.

jasrusable · 2022-10-27T09:22:21Z

@dougg0k I'm going to merge this PR for now because I'm happy with the changes otherwise. I'll do a bit more debugging locally and on GitHub Actions regarding this failing test before making a new release.

dougg0k added 3 commits September 5, 2022 21:26

add all changes

5277496

completely removed the need for lodash

d90e261

returned to es5

0b1faac

dougg0k added 5 commits September 7, 2022 12:05

upgraded ioredis to v5

8934f75

fixed and refactored data into a varible to avoid mistakes

c06a7c1

same refactoring in another place

46009fc

removed unneeded async and simplified redis quit

3459b20

more packages upgrades

8cde379

jasrusable requested changes Oct 13, 2022

View reviewed changes

dougg0k added 3 commits October 14, 2022 08:47

changed to npm

9798633

upgraded packages to latest, expect starting with p-

ee672df

changes ci files to npm

5c088e2

dougg0k requested a review from jasrusable October 14, 2022 11:58

falkenhawk reviewed Oct 14, 2022

View reviewed changes

changed to date-fns and added caret to deps

394499e

dougg0k requested review from falkenhawk and jasrusable and removed request for jasrusable and falkenhawk October 14, 2022 23:02

downgraded jest and changed tests to run on nodejs 12,14,16 instead

1d25d9b

upgraded packages and removed unused type

3bc6131

dougg0k requested review from falkenhawk and jasrusable and removed request for jasrusable and falkenhawk October 22, 2022 20:55

changed supported nodejs version to 14,16,18

cea0a00

returned to npm in benchmarks and examples

f63edce

jasrusable merged commit 75d7abf into conveyor-mq:master Oct 27, 2022

jasrusable mentioned this pull request Oct 27, 2022

Broken lua filepaths when transpiled/bundled #114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Many changes #166

Many changes #166

dougg0k commented Sep 6, 2022 •

edited

jasrusable commented Sep 7, 2022

dougg0k commented Oct 6, 2022

dougg0k commented Oct 13, 2022

jasrusable left a comment

jasrusable commented Oct 13, 2022

dougg0k commented Oct 13, 2022 •

edited

jasrusable commented Oct 14, 2022

dougg0k commented Oct 14, 2022

dougg0k commented Oct 14, 2022

jasrusable commented Oct 14, 2022

dougg0k commented Oct 14, 2022 •

edited

falkenhawk Oct 14, 2022

dougg0k Oct 14, 2022

dougg0k Oct 14, 2022

dougg0k commented Oct 22, 2022 •

edited

dougg0k commented Oct 22, 2022 •

edited

jasrusable commented Oct 22, 2022

dougg0k commented Oct 22, 2022

jasrusable commented Oct 25, 2022

dougg0k commented Oct 25, 2022 •

edited

dougg0k commented Oct 25, 2022 •

edited

dougg0k commented Oct 26, 2022 •

edited

jasrusable commented Oct 27, 2022

jasrusable commented Oct 27, 2022

Many changes #166

Many changes #166

Conversation

dougg0k commented Sep 6, 2022 • edited

jasrusable commented Sep 7, 2022

dougg0k commented Oct 6, 2022

dougg0k commented Oct 13, 2022

jasrusable left a comment

Choose a reason for hiding this comment

jasrusable commented Oct 13, 2022

dougg0k commented Oct 13, 2022 • edited

jasrusable commented Oct 14, 2022

dougg0k commented Oct 14, 2022

dougg0k commented Oct 14, 2022

jasrusable commented Oct 14, 2022

dougg0k commented Oct 14, 2022 • edited

falkenhawk Oct 14, 2022

Choose a reason for hiding this comment

dougg0k Oct 14, 2022

Choose a reason for hiding this comment

dougg0k Oct 14, 2022

Choose a reason for hiding this comment

dougg0k commented Oct 22, 2022 • edited

dougg0k commented Oct 22, 2022 • edited

jasrusable commented Oct 22, 2022

dougg0k commented Oct 22, 2022

jasrusable commented Oct 25, 2022

dougg0k commented Oct 25, 2022 • edited

dougg0k commented Oct 25, 2022 • edited

dougg0k commented Oct 26, 2022 • edited

jasrusable commented Oct 27, 2022

jasrusable commented Oct 27, 2022

dougg0k commented Sep 6, 2022 •

edited

dougg0k commented Oct 13, 2022 •

edited

dougg0k commented Oct 14, 2022 •

edited

dougg0k commented Oct 22, 2022 •

edited

dougg0k commented Oct 22, 2022 •

edited

dougg0k commented Oct 25, 2022 •

edited

dougg0k commented Oct 25, 2022 •

edited

dougg0k commented Oct 26, 2022 •

edited