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.

Sign up for GitHub

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

aem2doc parsing now done in da-collab #163

Merged
merged 4 commits into from
May 28, 2024
Merged

aem2doc parsing now done in da-collab #163

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec8c88f
Remove the aem2prose handling
bosschaert May 24, 2024
41ba5a5
Clear error after logging
bosschaert May 24, 2024
2bdf459
Merge branch 'main' into parsing-2
bosschaert May 27, 2024
f8f4417
Update da-editor.js
karlpauls May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions blocks/edit/da-editor/da-editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,18 @@ export default class DaEditor extends LitElement {
initIms().then(() => { this._imsLoaded = true; });
}

disconnectWebsocket() {
if (this.wsProvider) {
this.wsProvider.disconnect({ data: 'Client navigation' });
this.wsProvider = undefined;
}
}

disconnectedCallback() {
super.disconnectedCallback();
this.disconnectWebsocket();
}

async fetchVersion() {
const resp = await daFetch(this.version);
if (!resp.ok) return;
Expand Down Expand Up @@ -72,6 +84,7 @@ export default class DaEditor extends LitElement {

updated(props) {
if (!this._imsLoaded) return;
this.disconnectWebsocket();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

The way I read this is that the websocket will be disconnected for every update... and that seems a little worrisome / noisy.

Copy link
Contributor Author

@bosschaert bosschaert May 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

  1. Can add table.
  2. Can edit doc.
  3. Can edit doc with another user.
  4. Can switch docs via hashchange

etc.

Ideally this would funnel into Playwright so we don't have to do this stuff manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

@auniverseaway, should be fixed - can you approve again?

Copy link
Contributor Author

@bosschaert bosschaert May 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

if (!(props.has('version') || props.has('_versionDom'))) {
const prose = this.shadowRoot.querySelector('.da-prose-mirror');
prose.innerHTML = '';
Expand Down
71 changes: 13 additions & 58 deletions blocks/edit/prose/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import {
EditorState,
EditorView,
DOMParser,
Schema,
baseSchema,
history,
Expand All @@ -25,15 +24,13 @@ import {
yUndoPlugin,
yUndo,
yRedo,
prosemirrorToYXmlFragment,
} from 'da-y-wrapper';

// DA
import prose2aem from '../../shared/prose2aem.js';
import menu from './plugins/menu.js';
import imageDrop from './plugins/imageDrop.js';
import linkConverter from './plugins/linkConverter.js';
import { aem2prose, parse } from '../utils/helpers.js';
import { COLLAB_ORIGIN, getDaAdmin } from '../../shared/constants.js';
import { addLocNodes, getLocClass } from './loc-utils.js';

Expand All @@ -58,6 +55,8 @@ function addCustomMarks(marks) {
.addToEnd('contextHighlightingMark', contextHighlight);
}

// Note: until getSchema() is separated in its own module, this function needs to be kept in-sync
// with the getSchema() function in da-collab src/collab.js
export function getSchema() {
const { marks, nodes: baseNodes } = baseSchema.spec;
const withLocNodes = addLocNodes(baseNodes);
Expand Down Expand Up @@ -101,18 +100,6 @@ function pollForUpdates() {
}, 500);
}

// Apply the document in AEM doc format to the editor.
// For this it's converted to Prose and then applied to the current ydoc as an XML fragment
function setAEMDocInEditor(aemDoc, yXmlFragment, schema) {
const doc = parse(aemDoc);
const pdoc = aem2prose(doc);
const docc = document.createElement('div');
docc.append(...pdoc);
const parser = DOMParser.fromSchema(schema);
const fin = parser.parse(docc);
prosemirrorToYXmlFragment(fin, yXmlFragment);
}

function handleAwarenessUpdates(wsProvider, daTitle, win) {
const users = new Set();

Expand Down Expand Up @@ -148,45 +135,13 @@ export function createAwarenessStatusWidget(wsProvider, win) {
return daTitle;
}

export function handleYDocUpdates({
daTitle, editor, ydoc, path, schema, wsProvider, yXmlFragment, fnInitProse,
}, win = window, fnSetAEMDocInEditor = setAEMDocInEditor) {
let firstUpdate = true;
ydoc.on('update', (_, originWS) => {
if (firstUpdate) {
firstUpdate = false;

// Do the following async to allow the ydoc to init itself with any
// changes coming from other editors
setTimeout(() => {
const aemMap = ydoc.getMap('aem');
const current = aemMap.get('content');
const inital = aemMap.get('initial');
if (!current && inital) {
fnSetAEMDocInEditor(inital, yXmlFragment, schema);
}
}, 1);
}

const serverInvKey = 'svrinv';
const svrUpdate = ydoc.getMap('aem').get(serverInvKey);
if (svrUpdate) {
// push update from the server: re-init document
delete daTitle.collabStatus;
delete daTitle.collabUsers;
ydoc.destroy();
wsProvider.destroy();
editor.innerHTML = '';
fnInitProse({ editor, path });
return;
}

if (originWS && originWS !== wsProvider) {
const proseEl = win.view.root.querySelector('.ProseMirror');
const clone = proseEl.cloneNode(true);
const aem = prose2aem(clone);
const aemMap = ydoc.getMap('aem');
aemMap.set('content', aem);
function registerErrorHandler(ydoc) {
ydoc.on('update', () => {
const errorMap = ydoc.getMap('error');
if (errorMap && errorMap.size > 0) {
// eslint-disable-next-line no-console
console.log('Error from server', JSON.stringify(errorMap));
errorMap.clear();
}
});
}
Expand Down Expand Up @@ -226,12 +181,10 @@ export default function initProse({ editor, path }) {
}

const wsProvider = new WebsocketProvider(server, roomName, ydoc, opts);
const daTitle = createAwarenessStatusWidget(wsProvider, window);
createAwarenessStatusWidget(wsProvider, window);
registerErrorHandler(ydoc);

const yXmlFragment = ydoc.getXmlFragment('prosemirror');
handleYDocUpdates({
daTitle, editor, ydoc, path, schema, wsProvider, yXmlFragment, fnInitProse: initProse,
});

if (window.adobeIMS?.isSignedInUser()) {
window.adobeIMS.getProfile().then(
Expand Down Expand Up @@ -310,4 +263,6 @@ export default function initProse({ editor, path }) {

document.execCommand('enableObjectResizing', false, 'false');
document.execCommand('enableInlineTableEditing', false, 'false');

return wsProvider;
}
1 change: 1 addition & 0 deletions blocks/shared/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const DA_ADMIN_ENVS = {

const DA_COLLAB_ENVS = {
local: 'ws://localhost:4711',
stage: 'wss://stage-collab.da.live',
prod: 'wss://collab.da.live',
};

Expand Down
22 changes: 22 additions & 0 deletions test/unit/blocks/edit/da-editor/da-editor.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { expect } from '@esm-bundle/chai';

// This is needed to make a dynamic import work that is indirectly referenced
// from da-editor.js
const { setNx } = await import('../../../../../scripts/utils.js');
setNx('/bheuaark/', { hostname: 'localhost' });

const { default: DaEditor } = await import('../../../../../blocks/edit/da-editor/da-editor.js');

describe('da-editor', () => {
it('Test wsprovider disconnectedcallback', async () => {
const ed = new DaEditor();

const called = [];
const mockWSProvider = { disconnect: () => called.push('disconnect') };

ed.wsProvider = mockWSProvider;
ed.disconnectWebsocket();
expect(ed.wsProvider).to.be.undefined;
expect(called).to.deep.equal(['disconnect']);
});
});
93 changes: 0 additions & 93 deletions test/unit/blocks/edit/proseCollab.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,97 +73,4 @@ describe('Prose collab', () => {
awarenessOnCalled[0].f(delta2); // Call the callback function
expect(daTitle.collabUsers).to.deep.equal(['Anonymous', 'Anonymous', 'Joe Bloggs']);
});

it('Test YDoc firstUpdate callback', (done) => {
const ydocMap = new Map();
ydocMap.set('initial', 'Some intial text');

const ydocOnCalls = [];
const ydoc = {
getMap: (n) => (n === 'aem' ? ydocMap : null),
on: (n, f) => ydocOnCalls.push({ n, f }),
};

const setAEMDocCalls = [];
const fnSetAEMDoc = () => setAEMDocCalls.push('called');

pi.handleYDocUpdates({
daTitle: {},
editor: {},
ydoc,
path: {},
schema: {},
wsProvider: {},
yXmlFragment: {},
fnInitProse: () => {},
}, {}, fnSetAEMDoc);
expect(ydocOnCalls.length).to.equal(1);
expect(ydocOnCalls[0].n).to.equal('update');

ydocOnCalls[0].f();
setTimeout(() => {
expect(setAEMDocCalls).to.deep.equal(['called']);

// the function call again, it should not perform any action this time
ydocOnCalls[0].f();
setTimeout(() => {
expect(setAEMDocCalls).to.deep.equal(
['called'],
'First update code should only be called once',
);
done();
}, 200);
}, 200);
});

it('Test YDoc server update callback', () => {
const daTitle = {
collabStatus: 'yeah',
collabUsers: 'some',
};
const editor = {};

const ydocMap = new Map();
ydocMap.set('svrinv', 'Some svrinv text');

const ydocCalls = [];
const ydocOnCalls = [];
const ydoc = {
getMap: (n) => (n === 'aem' ? ydocMap : null),
destroy: () => ydocCalls.push('destroy'),
on: (n, f) => ydocOnCalls.push({ n, f }),
};

const wspCalls = [];
const wsp = { destroy: () => wspCalls.push('destroy') };

const initProseCalls = [];
const mockInitProse = () => initProseCalls.push('init');

pi.handleYDocUpdates({
daTitle,
editor,
ydoc,
path: {},
schema: {},
wsProvider: wsp,
yXmlFragment: {},
fnInitProse: mockInitProse,
}, {}, () => {});
expect(ydocOnCalls.length).to.equal(1);
expect(ydocOnCalls[0].n).to.equal('update');

expect(daTitle.collabStatus).to.equal('yeah', 'Precondition');
expect(daTitle.collabUsers).to.equal('some', 'Precondition');

// Calls server invalidation
ydocOnCalls[0].f();

expect(daTitle.collabStatus).to.be.undefined;
expect(daTitle.collabUsers).to.be.undefined;
expect(ydocCalls).to.deep.equal(['destroy']);
expect(wspCalls).to.deep.equal(['destroy']);
expect(initProseCalls).to.deep.equal(['init']);
expect(editor.innerHTML).to.equal('');
});
});
Loading