aem2doc parsing now done in da-collab #163
Conversation
bosschaert
changed the title
Also cleans up old websockets on navigation
|
The Playwright tests should start parsing once adobe/da-collab#38 is merged. |
blocks/edit/da-editor/da-editor.js
Outdated
| @@ -72,6 +84,7 @@ export default class DaEditor extends LitElement { | |||
|
|
|||
| updated(props) { | |||
| if (!this._imsLoaded) return; | |||
| this.disconnectWebsocket(); | |||
There was a problem hiding this comment.
I don't understand why this is here, but I guess @bosschaert and @karlpauls you both do?
I will probably try to do some collab tests with multiple users as that is something I haven't done yet.
There was a problem hiding this comment.
The way I read this is that the websocket will be disconnected for every update... and that seems a little worrisome / noisy.
There was a problem hiding this comment.
What we were seeing was that when you move to a different document (for example by typing the doc name in the browser location bar), that connections to the previous document were still going on because the websocket wasn't closed. This caused errors in the log, but also (potentially a lot of) unnecessary websocket communication.
IIRC this is because the document URL didn't change ( http://localhost:3000/edit#/da-sites/da-status/bosschae/doc1 vs http://localhost:3000/edit#/da-sites/da-status/bosschae/doc2 ) is really just an anchor change, the location is still the same.
So, no the websocket won't get disconnected for every update, but it will get disconnected when the anchor is updated, i.e. we're moving to a different document.
There was a problem hiding this comment.
Actually, I think @auniverseaway is correct - this looks wrong. It should only disconnect if initProse is called. In other words, I think it needs to be moved inside the if statement one line down.
There was a problem hiding this comment.
@bosschaert, I think you latest merge did mess this up. You never assign the wsProvider and this disconnect should be in the if I think.
There was a problem hiding this comment.
@bosschaert I didn't think of the doc change...
This makes me think we need to get a wiki of E2E test steps for our own reminders of what to test for these high impact changes.
- Can add table.
- Can edit doc.
- Can edit doc with another user.
- Can switch docs via hashchange
etc.
Ideally this would funnel into Playwright so we don't have to do this stuff manually.
There was a problem hiding this comment.
@auniverseaway, should be fixed - can you approve again?
There was a problem hiding this comment.
Thanks for fixing @karlpauls yes it got messed up in the merge.
I created a Playwrighte test wiki here: https://github.com/adobe/da-live/wiki/TODO-%E2%80%90-Playwright-tests
Also cleans up old websockets on navigation
Description
da-collab now does its own aem2doc and vice versa parsing. This PR is to accommodate this on the client side.
It also contains a bugfix to close the current websocket if the client navigates to a different document.
Related Issue
This is the client-side change for
adobe/da-collab#38
How Has This Been Tested?
Types of changes
Checklist: