Monaco Adapter: Rewrite from scratch in ES6+ #325

0xTheProDev · 2019-01-14T10:05:24Z

Created only those APIs that are been called either by
Firepad or EditorClient.

Syncing between editors and cursors.

Updated package-lock, found 7 vulnerability with breaking
change update

Removed depenencies and function prototypes created while
transpiling from Coffescript code for Ace. Rewritten specific to
Monaco Editor and it's API standards.

Add Build task to convert ES6+ codes into ES5 using Babel.

Add Testcases to verify changes from Monaco editor onChange
event handler to OT.js Text Operations.

Ref: #324
Closes: #313

Signed-off-by: Progyan Bhattacharya Progyan@hackerrank.com

0xTheProDev · 2019-01-16T16:42:12Z

URGENT
@mikelehen Can you please review this?

samtstern · 2019-01-16T17:44:56Z

@Progyan1997 thanks for the contribution! This is a very large change and it will be tough to review. I see your last message marked this URGENT, can you explain?

In general can you explain the benefit to Firepad developers of converting the Monaco adapter to ES6? Apologies if this seems obvious, I have not been involved with Firepad for long so it's not clear to me.

0xTheProDev · 2019-01-17T07:59:03Z

@samtstern There were couple of bugs regarding changing onChange event in Monaco to OT.js Text Operations, and in syncing of cursor position and selection between pads. These bugs sometimes were hard to reproduce and fix. Also existing codebase was a conversion of Ace-Adapter with couple of changes for Monaco and had residues of converted coffee code which was not required. So to fix existing issues and specialising Adapter for Monaco, the rewrite has been done.

ES6+ standard is just a design choice that I made as it is latest standard. I have also added build tasks in grunt to facilitate ES5 support for old browsers and tests as well.

mikelehen · 2019-01-28T00:34:14Z

@Progyan1997 Thanks for the PR! As Sam said, this is fairly large, and I'm not very familiar with monaco in the first place, so it's difficult for me to review this and ensure it'll be an improvement for people using the monaco support.

I wonder if any of the folks who have worked on the monaco support in the past would be interested in taking a look and/or trying it out (e.g. @pranjaltale16, @Cnidarias, @ddunkin, @oliverlin, or @jbchavez19 )?

0xTheProDev · 2019-01-28T07:24:44Z

@mikelehen @samtstern Why don't you guys perform build and launch monaco.html from example to see the improvements?

samtstern · 2019-01-29T19:48:51Z

@Progyan1997 the problem is that it's very hard to just click-test a complete re-write of code like this, the possibility of missing bugs is too high if we're not comfortable with our review.

Another option would be to split this into two PRs:
#1 - fix the two bugs you're targeting
#2 - convert code to ES6

Separating those would make it much easier to have a high level of confidence.

Cnidarias · 2019-01-29T19:52:21Z

I can have a look at what was done coming this weekend, but I am no expert on the subject as well.

zach-karat · 2019-02-05T00:07:39Z

I support merging this PR in some form (or splitting it into two).

I have been testing monaco-adapter in my own project and encountered a few bugs that make it almost unusable in a few key situations (such as tabbing multiple lines and commenting out multiple lines). I have been testing locally and this change fixes all the bugs I am aware of in the adapter.

samtstern · 2019-02-08T19:23:13Z

@zach-karat thanks for your PR (#327). Are there more bug fixes contained in this PR that were not present there?

zach-karat · 2019-02-08T19:24:50Z

@samtstern Not that I know of, but I did not review every file. @Progyan1997 ?

0xTheProDev · 2019-02-14T19:00:46Z

@samtstern @mikelehen @zach-karat I created an updated PR following current ES5 syntax, so that ES6 rewrite overhead can be avoided in review process. Thanks.

Additional Fixes: Proper undo-redo, multiline commenting/uncommenting, sharing selection and cursor.

samtstern · 2019-02-15T16:18:30Z

@Progyan1997 thank you for taking the time to simplify! I will review this when I get a chance (since I am not a firepad expert it will take me longer than it should) and hopefully we will get it merged.

samtstern

I mostly have some minor concerns (in comments). How confident are you that the tests / your own testing ensure this change is correct?

I really like the increased code readability of the new code but I am having trouble keeping enough in my head (without knowing anything about Monaco) to say that I can guarantee the same behavior just by reading the code.

samtstern · 2019-02-21T23:57:29Z

lib/monaco-adapter.js


+  /** Add style rules only once */
+  if (this.addedStyleRules.indexOf(clazz) === -1) {


In the old implementation this was an object {} which made it easier to check for membership, why move to an array and indexOf?

Because I rewrote in ES6 at first, and using [] is not regarded as best practice. I will vouch we keep it like this, so that most of the responsibility moves toward language and library than the coder.

samtstern · 2019-02-22T00:04:04Z

lib/monaco-adapter.js

-      start = this.monacoModel.getOffsetAt(selection.getStartPosition());
-      end = this.monacoModel.getOffsetAt(selection.getEndPosition());
-    } catch (_error) {
-      e2 = _error;


What happened to this error case?

I didn't find any situation where statements above will throw any error.

samtstern · 2019-02-22T00:10:45Z

lib/monaco-adapter.js

+   * @param {function[]} callbacks - Set of callback functions
+   */
+  MonacoAdapter.prototype.registerCallbacks = function registerCallbacks(callbacks) {
+    this.callbacks = Object.assign({}, this.callbacks, callbacks);


Can you explain why the move to Object.assign from straight assignment?

So that if new param doesn't have action for some event, we can use the existing ones.

samtstern · 2019-02-22T00:12:38Z

lib/monaco-adapter.js

-        offsetLength = offsetLength + (pair[0].targetLength - pair[0].baseLength);
-        this.trigger.apply(this, ['change'].concat(__slice.call(pair)));
-      }.bind(this))
-      return this.grabDocumentState();


So was this return value unused before?

lib/monaco-adapter.js

samtstern · 2019-02-22T00:15:39Z

lib/monaco-adapter.js

    }
-    this.grabDocumentState();


Do we want to call this at the end of applyOperation?

No, related state transition has already been taken care of

**This is a modified commit of earlier PR** Created only those APIs that are been called either by Firepad or EditorClient. Main fix is for syncing between multiple pads and cursors. Removed depenencies and function prototypes created while transpiling from Coffescript code for Ace. Rewritten specific to Monaco Editor and it's API standards. Capable of handling multi-line operations and flexible with undo-redo. Add Testcases to verify changes from Monaco editor onChange event handler to OT.js Text Operations. Reduced scope for internal methods, so that they are inaccessible from outside world. There were couple of bugs regarding changing `onChange` event in Monaco to `OT.js` Text Operations, and in syncing of cursor position and selection between pads. These bugs sometimes were hard to reproduce and fix. Also existing codebase was a conversion of `Ace-Adapter` with couple of changes for Monaco and had residues of converted `coffee` code which was not required. Ref: #324 (Transpiled down to ES5) Closes: #313 Signed-off-by: Progyan Bhattacharya <Progyan@hackerrank.com>

samtstern · 2019-02-22T23:54:54Z

@zach-karat have you tried this change since #327 was merged?

zach-karat · 2019-02-25T20:03:04Z

@samtstern I haven't had a chance to test the latest version yet, but I can try to find time to merge and test.

samtstern · 2019-02-25T20:22:19Z

@zach-karat no worries I don't want to put extra burden on you was just curious if you'd already tried it, thanks!

samtstern · 2019-02-28T19:36:41Z

Ok I have taken a few more looks at this and while I am still a little uncomfortable with my lack of understanding, I think it's a shame to let such a well thought out contribution to a community-managed project go unmerged.

So we can merge this, cut a release, and fix new issues as they come up.

0xTheProDev · 2019-03-13T10:30:19Z

@samtstern Hey just wanted to know when the next release is going to be published?

…rebaseExtended#325)

zach-karat mentioned this pull request Feb 5, 2019

Fix incorrect offset handling in Monaco adapter. #327

Merged

0xTheProDev closed this Feb 14, 2019

0xTheProDev reopened this Feb 14, 2019

samtstern self-assigned this Feb 15, 2019

samtstern reviewed Feb 22, 2019

View reviewed changes

samtstern merged commit e39b4c1 into FirebaseExtended:master Feb 28, 2019

samtstern added a commit that referenced this pull request Feb 28, 2019

Update changelog for #325

61c592a

samtstern mentioned this pull request Feb 28, 2019

Catch failures to sync Firebase, so Firepad clients can handle them. #329

Closed

0xTheProDev deleted the develop branch February 28, 2019 21:21

mfont-gh pushed a commit to mfont-gh/firepad that referenced this pull request Jun 3, 2019

Monaco Editor Support on Firebase: Update on API internal changes (Fi…

6fc2172

…rebaseExtended#325)

mfont-gh pushed a commit to mfont-gh/firepad that referenced this pull request Jun 3, 2019

Update changelog for FirebaseExtended#325

90db268

jgeggatt mentioned this pull request Mar 23, 2022

fix(monaco): make undo/redo actually work CoderPad/firepad#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Monaco Adapter: Rewrite from scratch in ES6+ #325

Monaco Adapter: Rewrite from scratch in ES6+ #325

0xTheProDev commented Jan 14, 2019 •

edited

0xTheProDev commented Jan 16, 2019

samtstern commented Jan 16, 2019

0xTheProDev commented Jan 17, 2019 •

edited

mikelehen commented Jan 28, 2019

0xTheProDev commented Jan 28, 2019

samtstern commented Jan 29, 2019

Cnidarias commented Jan 29, 2019

zach-karat commented Feb 5, 2019

samtstern commented Feb 8, 2019

zach-karat commented Feb 8, 2019

0xTheProDev commented Feb 14, 2019 •

edited

samtstern commented Feb 15, 2019

samtstern left a comment

samtstern Feb 21, 2019

0xTheProDev Feb 22, 2019

samtstern Feb 22, 2019

0xTheProDev Feb 22, 2019

samtstern Feb 22, 2019

0xTheProDev Feb 22, 2019

samtstern Feb 22, 2019

0xTheProDev Feb 22, 2019

samtstern Feb 22, 2019

0xTheProDev Feb 22, 2019

samtstern commented Feb 22, 2019

zach-karat commented Feb 25, 2019

samtstern commented Feb 25, 2019

samtstern commented Feb 28, 2019

0xTheProDev commented Mar 13, 2019


		/** Add style rules only once */
		if (this.addedStyleRules.indexOf(clazz) === -1) {

Monaco Adapter: Rewrite from scratch in ES6+ #325

Monaco Adapter: Rewrite from scratch in ES6+ #325

Conversation

0xTheProDev commented Jan 14, 2019 • edited

0xTheProDev commented Jan 16, 2019

samtstern commented Jan 16, 2019

0xTheProDev commented Jan 17, 2019 • edited

mikelehen commented Jan 28, 2019

0xTheProDev commented Jan 28, 2019

samtstern commented Jan 29, 2019

Cnidarias commented Jan 29, 2019

zach-karat commented Feb 5, 2019

samtstern commented Feb 8, 2019

zach-karat commented Feb 8, 2019

0xTheProDev commented Feb 14, 2019 • edited

samtstern commented Feb 15, 2019

samtstern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samtstern commented Feb 22, 2019

zach-karat commented Feb 25, 2019

samtstern commented Feb 25, 2019

samtstern commented Feb 28, 2019

0xTheProDev commented Mar 13, 2019

0xTheProDev commented Jan 14, 2019 •

edited

0xTheProDev commented Jan 17, 2019 •

edited

0xTheProDev commented Feb 14, 2019 •

edited