-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Server: Resolves #9355: Add sync debug page to Joplin Server #9368
Server: Resolves #9355: Add sync debug page to Joplin Server #9368
Conversation
@@ -20,6 +20,7 @@ | |||
"scripts": { | |||
"tsc": "tsc --project tsconfig.json", | |||
"watch": "tsc --watch --preserveWatchOutput --project tsconfig.json", | |||
"build": "yarn run tsc", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently needed because yarn run tsc
is run after yarn workspaces foreach --verbose --interlaced --topological run build
. A script run during yarn ... run build
, however, now relies on utils
being compiled.
A similar line is added in #9360.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer we keep the existing order - install, build, tsc to keeps things more predictable. So far we managed to do this, so we can probably find a solution here too? In some cases I think we use ts-node to run TypeScript files before they've been built, which would be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently required to use execCommand
in gulpfile.js
.
There does seem to be a way to run gulpfile
s with ts-node
, but if we switch to a different build method, this shouldn't be necessary (see comment below).
It also might not be sufficient — build.ts
is run with ts-node
in #9360 but still seems to be unable to import execCommand
on a first build. I think it's related to @joplin/utils
compiling JavaScript into a dist/
directory and not exporting the TypeScript?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we're also running tsc
during build
for packages/react-native-saf-x
and packages/fork-htmlparser2
.
I'm not sure using ts-node
would help here — #9360 uses ts-node
and is experiencing the same issue.
"test": "jest --verbose=false", | ||
"test-ci": "yarn test", | ||
"test-debug": "node --inspect node_modules/.bin/jest -- --verbose=false", | ||
"clean": "gulp clean", | ||
"populateDatabase": "JOPLIN_TESTS_SERVER_DB=pg node dist/utils/testing/populateDatabase", | ||
"stripeListen": "stripe listen --forward-to http://joplincloud.local:22300/stripe/webhook", | ||
"watch": "tsc --watch --preserveWatchOutput --project tsconfig.json" | ||
"watch": "gulp watch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of moving this to gulp because that's yet another level of indirection. And we used to use gulp for watching and it caused various issues so I'd prefer if we stay as close as possible to the tsc binary for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some options:
- Do compilation with TypeScript project references, as was done in an earlier commit.
- Benefits:
- Calling
tsc
for watching/building only requires one call (e.g.tsc --build --watch --preserveWatchOutput
ortsc --build
).
- Calling
- Drawbacks:
- Requires building
.d.ts
files in the referenced project. - There doesn't seem to be a way to specify the root
tsconfig.json
file (other than by changing the current working directory). As theserver
package currently specifies atsconfig.json
forbuild
/watch
commands, I suspect that specifying the directory with the roottsconfig.json
is (or was) important.
- Requires building
- Benefits:
- Current method — call
tsc
withexecCommand
fromgulp
(or similar method: Calltsc
from acompile-typescript.ts
file run byts-node
).- Benefits:
- Can watch both projects at the same time
- Runs
tsc
for both projects during thetsc
step (rather than duringbuild
) - Works on non-bash-like shells (as opposed to
tsc --watch --project project1 & ; tsc --watch --project project2
)
- Drawbacks:
- Requires modifying
@joplin/utils
to makeexecCommand
available during thebuild
step or modifying@joplin/utils
to also export TypeScript files and moving to a TypeScript gulp configuration file. (Alternatively, re-implementingexecCommand
.)
- Requires modifying
- Benefits:
- Create a separate
watch-public
command for thepublic/
directory and compile both withtsc --project ./tsconfig.jsoon && tsc --project ./public/tsconfig.json
.- Benefits:
- Simple — doesn't require running
tsc
from JavaScript or significantly changing build flags.
- Simple — doesn't require running
- Drawbacks:
yarn run watch
won't watch the public directory.
- Benefits:
- Use
webpack
orrollup
and abuild-bundled-js
command that runs duringyarn build
.- Benefits:
- Allows
import
statements in TypeScript that will be built. - Similar to how JavaScript for WebViews, etc. are bundled in other packages (e.g.
app-mobile/
)
- Allows
- Drawbacks:
- More complicated to set up
- Easier to unintentionally bundle large amounts of code
- Benefits:
@@ -20,6 +20,7 @@ | |||
"scripts": { | |||
"tsc": "tsc --project tsconfig.json", | |||
"watch": "tsc --watch --preserveWatchOutput --project tsconfig.json", | |||
"build": "yarn run tsc", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer we keep the existing order - install, build, tsc to keeps things more predictable. So far we managed to do this, so we can probably find a solution here too? In some cases I think we use ts-node to run TypeScript files before they've been built, which would be fine.
@personalizedrefrigerator, I was looking at this PR again. Most of the implementation is on client-side however that doesn't really seem necessary? For example, checking if an item is on the server, or part of that initial diff, can be done server-side. In fact in general we mostly do things server-side in Joplin Server/Cloud as it's easier to manage and test. Is there any reason why we should do all this client-side? |
It was originally done server-side and generated a single, long debug report. However, such checks can take a long time, particularly with a large number of notes. (The code was similar to the disabled changes route). Doing this client-side takes advantage of existing protections against too many requests (and thus makes denial of service attacks more difficult). |
I think this pull request would be helpful in resolving this sync issue. Would it make sense to implement the sync debug page in JavaScript instead of TypeScript? Doing so would simplify this pull request. |
For now, I'm closing this as not planned. If necessary, this pull request can be re-opened in the future. |
Summary
Adds a new
/sync_debug
route to Joplin Server that provides the following information:diff
To take advantage of existing rate-limiting code, information is fetched from a client-side script rather than on the server.
Resolves #9355.
Notes
Joplin Server doesn't seem to have a bundler or other workflow set up for converting TypeScript to client-side JavaScript. As such, most of the new code is in JavaScript, rather than TypeScript.
Screenshot
Testing plan
http://localhost:22300/sync_debug
in a browserCheck if an item is present on the server
input box and pressSubmit
Check if an item is in the diff used for an initial sync
input box and pressSubmit