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

Fixes and additions to editor API #4780

Merged
merged 9 commits into from
Oct 10, 2023
Merged

Fixes and additions to editor API #4780

merged 9 commits into from
Oct 10, 2023

Conversation

mejo-
Copy link
Member

@mejo- mejo- commented Sep 7, 2023

📝 Summary

This is required for Collectives to migrate to the editor API. Changes are separated by commits.

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits

@mejo- mejo- added bug Something isn't working 3. to review labels Sep 7, 2023
@cypress
Copy link

cypress bot commented Sep 7, 2023

3 failed and 2 flaky tests on run #12080 ↗︎

3 137 18 0 Flakiness 2
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Fixes and additions to editor API
Project: Text Commit: 1527f9a668
Status: Failed Duration: 32:35 💡
Started: Sep 11, 2023 3:02 PM Ended: Sep 11, 2023 3:34 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

src/editor.js Outdated Show resolved Hide resolved
@juliusknorr

This comment was marked as resolved.

@mejo- mejo- mentioned this pull request Sep 11, 2023
@mejo-

This comment was marked as resolved.

@@ -268,6 +268,7 @@ class SyncService {
this.autosave.clear()
} catch (e) {
logger.error('Failed to save document.', { error: e })
throw e
Copy link
Member

Choose a reason for hiding this comment

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

I thought I commented that already, but i feel this might be a risky change if the save fails during other code parts calling this method. Can we safeguard that other callers catch this error then and handle it gracefully?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. On the other hand for the API usage it would be really handy to be able to detect failures when saving 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

If we can get this in and backported more quickly without the throw e change, I'm fine with postponing this one 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

I had a look at all occasions of .save(), .autosave() and .forceSave(). Seems to be mostly places where throwing an error should be save. But maybe it's better to remove this potentially dangerous change from this PR that is supposed to be backported and move it to a seperate PR that doesn't get backported.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did this now.

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Looks good besides the one comment

mejo- added 5 commits October 10, 2023 15:46
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
@mejo- mejo- force-pushed the fix/editor_api branch 2 times, most recently from 90f71d5 to fb1f4e0 Compare October 10, 2023 13:50
Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

reviewed and looks good to me.

Did not investigate julius' comment on the trow in SyncService.js yet. Is that needed / related?

mejo- added 4 commits October 10, 2023 16:31
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
@mejo-
Copy link
Member Author

mejo- commented Oct 10, 2023

Ready for review from my side now.

@mejo- mejo- merged commit fede64b into main Oct 10, 2023
21 checks passed
@mejo- mejo- deleted the fix/editor_api branch October 10, 2023 14:46
@mejo-
Copy link
Member Author

mejo- commented Oct 10, 2023

/backport to stable27

@mejo-
Copy link
Member Author

mejo- commented Oct 10, 2023

/backport to stable26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants