Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
BC-5736 - Redis for tldraw #4542
BC-5736 - Redis for tldraw #4542
Changes from 250 commits
f11d39f
6cb919e
9be6bf4
b2fd397
7eecf87
a9037cf
336de77
7475bf5
55ada6e
08ca974
89c7816
31b4f20
bcd3cbe
dc970fc
491bcce
4bc510f
6b16280
fcfb492
8a73d1e
bddb1e4
334b811
ecd2e16
62a8676
749e606
f98dcf6
b12eda6
785714d
962bbc2
3778e89
b84f001
981b677
23bc570
044ee76
e234a64
13cb011
70b82d2
a97b0a1
2c590b4
079ca95
a5c55c1
0f29122
01127a3
3621bc1
cd115b0
fb75138
778ac27
cdd36de
b78de7b
e454d80
8e3c893
63b9623
ccb48d0
0f756b2
ad2255f
392e9a6
3e0ca98
ca6348d
36c3a25
f87bd99
1f2b628
f3ec115
6e1499c
a81e432
d041766
b797efa
5976c90
9e1e044
b2c5c64
20f370d
a67ace0
5c7b4cb
d82a5b4
561c5a3
e6f426b
f908ca3
12f654f
2289de2
217d1b3
3dca940
eb4442f
1ff203e
7034c55
7fc4561
346b9cd
155ade0
34023fe
edff1ab
403a4f5
8a52067
522bffe
419a65f
ea25057
e8bbedc
23b6550
241f6c7
f50b253
dedd070
b7e0740
698ea6b
2684a5d
768eb17
f85ce21
af0e07e
36548f1
29e873c
5b7f726
2ab745a
0077b02
b79857a
052fb37
0e7df0a
42ebc54
53d8d15
8d1d8af
10f25a8
1ef4262
fedd7c7
668aeaf
f46359a
344dc2b
67e7f8b
eb50a5a
7c3f6a1
0b403e3
66d613c
39d8c61
3ee17c6
5f28d42
77544e0
862f2ef
0b0e05d
64d2b4d
0d14e90
f5986e4
060d31b
05278f5
8fb2451
a7f2c36
9f86f19
a9ab05c
fc44205
880e087
1408854
043a329
e7002ed
f061629
472ff24
b7085fe
930fe13
a043541
7db1030
41e98be
75251c6
e8417cd
803ef68
683d9c2
4a35e54
a3af533
7f1754d
4c0243e
1e5fbcd
dc039d2
92bb2d2
a981898
8d844bd
ee8b497
086eae1
e12d193
4128a4a
a559a04
8ff96ec
e951ada
3e9a6a6
f3d9edd
ff5adde
11f67bb
8d0071b
1fd16f0
c6c696f
5dd32b9
d0fb99e
a320dd7
64c9dc4
ef9f42f
3fff2f1
8ab87a5
eaef660
81eccdf
b51330a
4fd5200
f635f8e
e46dac3
485e7ff
919f662
f0bdd77
03877b0
6d0b8cc
8b6c3b4
88e5d63
7d2f04a
f390dfc
0ff9dc3
c26084e
5528da8
1710bc7
f1c137c
ca7b607
43fc03e
3f9dbf7
565a02c
e6781a9
35e5d38
0a2a508
8418ee7
10d0a58
4ecde56
c90337c
a79fa43
a25b30b
ee70791
21b0b0d
ecf7bb7
686f248
1f22f84
4856615
d5f8026
fe7f7b5
dd3f1e3
f3039f9
0335c47
634c3a6
5edb111
8f93b92
047decf
a6daad6
5770b49
4ba46fd
fb7c431
cd5499b
563a6fe
a7fea9a
1c22a21
7c2a8ec
c14a101
4b750cb
977610a
dad583b
daa869c
c8d7acb
b58d12e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Please add the default to configs. If it is for local setup only than use /config/development.json. If it is the default for production too, then it can be added as default to default.schema.json.
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 actually only need to set this for tests but I am not sure if there is a better way.
If I set this up in /config/test.json the feathers test will fail because it will try to connect to Redis and crash.
Cannot set it up in default.schema.json as there is some logic that works specifically if this env var is not defined.
Could not set this inside tests either as suite will not even start when env var is used but not defined.
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.
Either we make the redis required for the server (including local), or we have to make sure the server can start without it.
Like this, you will have undefined behavior. As you mentioned, there is logic that is accessing this value to decide stuff (to activate the autologout), and im not sure what will happen there if you have a value for the redis url without there actually being a redis behind it. Making the Redis required is a bigger decision, that should be taken into the arc chapter if its necessary.
Why does your suite not start without redis? it would also make sense to confirm the behavior of your code (that its disabled) when there is no redis, as that is a valid case at the moment. So I think we need to find a solution to this anyway.
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.
If redis is required for tldraw, I think its fine to fail (crash) during startup if tldraw is enabled and there is no redis URI.
You can make this clearer by adding a restriction to the schema, but not strictly necessary.
But that doesnt help for your test case. I am however not aware of any checks jest makes before running a testsuite, at least the beforeAll should run, and you should be able to mock the configuration before creating the test module... But tbh I havent tried that myself. If you are having trouble, please point me to a specific case then maybe I can investigate it a bit.
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.
Redis is indeed required for tldraw and it will crash without REDIS_URI, just how you described.
Test suite will fail with:
Configuration Error: The configuration key 'REDIS_URI' has been used, but it was not defined!
Redis is not actually needed in tests, just any value needs to be set on this variable but I'm not sure where else to do this. Maybe I'm just missing something or the tests are not set up correctly?
One idea I had was to introduce new variable TLDRAW_REDIS_URI which would have the same url but in this case I could just set it up on config/test.json without worrying that other tests relying on REDIS_URI will fail.
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.
wrote you a longer message in rocketchat with suggestions
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.
We do not want the jest mocks and spys if we can avoid it. Please check
import { createMock, DeepMocked } from '@golevelup/ts-jest';
DeepMocked
In this case it maybe valid to use jest if you do not want to mock each method of service.
But one point is confusing for me. If this is the do than it should nothing know about each service. It can be used inside of services.
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.
Please think about restructor of the code, to avoid adding the services to this do.
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.
done
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.
Why not throw up this error to the point that execute the destroy() or this lines of code ? Currently it do not work like i expect it.
Edit: The part that is added in constructor should be outsource to a method that can be called and the caller of this method can be handled the error.
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.
You are right, every single method of WsSharedDocDo was calling TldrawWsService so I moved all the logic there.
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.
Can you please move the new TldrawDrawing behind a test factory with default values than is placed in modules/tldraw/testing/* . Can you also work with bson package new ObjectId().toHexString() for each id if it is a MongoDB objectId instead of generic string.
This factory can use existing production factories if it exists but pass default testing values to it.
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.
done
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.
If it is not a number it should be undefined?
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.
yes, it can be undefined