Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve sync events #1290

Merged
merged 6 commits into from
Apr 13, 2023
Merged

Improve sync events #1290

merged 6 commits into from
Apr 13, 2023

Conversation

raucao
Copy link
Member

@raucao raucao commented Mar 26, 2023

  • Add "completed" status to sync-done
    A sync can be done with uncompleted tasks from errored requests. It will then continue during the next periodic sync, trying to fetch the errored items again. However, for UI integration it is useful to know whether the full sync process is completed or not, so it doesn't have to show sync finishing and re-starting several times throughout a long process with many items, which may involve a few attempts here and there.
  • Add number of remaining tasks in queue to sync-req-done
    This is just an indicator of overall status, since the internal queue only ever holds 100 items at most. However, for UI integration, it can be useful to know if there's a larger queue left. For example to show a dedicated sync status, as opposed to hiding it in order to not inform the user about small, relatively immediate sync procedures.

* Add "completed" status to `sync-done` (can finish with errored tasks)
* Add number of remaining tasks in queue to `sync-req-done`
@raucao raucao marked this pull request as ready for review March 29, 2023 15:35
@raucao raucao requested review from DougReeder and a team March 29, 2023 15:35
@raucao
Copy link
Member Author

raucao commented Mar 29, 2023

I added a fix for a subtle bug in the finishTask function, which made my new tests fail for no obvious reason: d740c0c

@DougReeder
Copy link
Contributor

The changes look good, but when I run npm ci and npm run test:mocha with this branch under Node 14 or 16, I get the error:

TSError: ⨯ Unable to compile TypeScript:
test/helpers/location.ts:90:5 - error TS2322: Type 'Document' is not assignable to type 'never'.

90 global["document"] = {} as Document;
~~~~~~~~~~~~~~~~~~

I don't get that error with the master branch; I'm not certain what's different.

@raucao
Copy link
Member Author

raucao commented Mar 30, 2023

Thanks. I did not change anything in that file, so it's unlikely that this error is related to my changes.

What's also weird is that GitHub didn't run the test actions for the last couple of commits. It still works fine for me on node 16 locally.

@raucao
Copy link
Member Author

raucao commented Apr 10, 2023

@DougReeder Could you try removing your node_modules folder and re-installing all modules from npm?

@DougReeder
Copy link
Contributor

I deleted node-modules and updated npm to 9.6.4, and now when I run npm ci or npm i I get these errors:

npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR!
npm ERR! While resolving: typedoc@0.19.2
npm ERR! Found: typescript@4.8.4
npm ERR! node_modules/typescript
npm ERR! dev typescript@"^4.8.3" from the root project
npm ERR! peer typescript@"*" from ts-loader@8.4.0
npm ERR! node_modules/ts-loader
npm ERR! dev ts-loader@"^8.4.0" from the root project
npm ERR! 2 more (ts-node, tsutils)
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer typescript@"3.9.x || 4.0.x" from typedoc@0.19.2
npm ERR! node_modules/typedoc
npm ERR! dev typedoc@"^0.19.2" from the root project
npm ERR!
npm ERR! Conflicting peer dependency: typescript@4.0.8
npm ERR! node_modules/typescript
npm ERR! peer typescript@"3.9.x || 4.0.x" from typedoc@0.19.2
npm ERR! node_modules/typedoc
npm ERR! dev typedoc@"^0.19.2" from the root project

As noted, this PR doesn't change the dependencies, so I don't see how this issue could have been introduced by it. However. until I figure out what's going on, I can't sign off on this PR. :-(

@raucao
Copy link
Member Author

raucao commented Apr 12, 2023

Ah. You need to use the --force flag with those currently

https://remotestoragejs.readthedocs.io/en/latest/contributing/building.html#setup

See also #1276

Copy link
Contributor

@DougReeder DougReeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing tests; that gives me a lot more confidence.

@@ -932,7 +932,7 @@ class Sync {
log('[Sync] wireclient rejects its promise!', task.path, task.action, err);
return this.handleResponse(task.path, task.action, { statusCode: 'offline' });
})
.then(completed => {
.then(async (completed) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to have finishTask be async and use await throughout, but that can wait for a code cleanup.

Copy link
Member Author

@raucao raucao Apr 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there are a lot of places where we will want to refactor using async/await, since it just wasn't available when the code was first written. In fact, we even had to use a library for promises at the time, because promises were the hot new thing while everyone was using callbacks still. ;)

@raucao raucao merged commit f28208a into master Apr 13, 2023
@raucao raucao deleted the feature/improve_sync_events branch April 13, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants