changes are duplicated (and first copy has no codelens) when going straight to Apply Changes

[gordonwoodhull]

Once we load the Monaco library on time #2634, we see an extra copy of the changes, with no codelens, then some blank space, and then the copy with the codelens.

Second time loading Apply Changes, they are all there.

[jameesy]

Think it might be worth mentioning that this error seems to be duplicating my output. In this instance you can see the same lines are repeated, once with codelens' and once without.

Overwhelmingly this can be replicated on the first run through from the tree. This has only occurred also since I synced the main repo to the Mango repo this morning and I have never seen this particular issue prior to this even though I have solely been using the "Merge Here' feature since implementing it.

[jameesy]

Funnily enough the original require statement seems to have disappeared from the codebase which seems to be causing this issue and would explain why I hadn't seen this before. Not sure where the original statement went but this seems to fix this for me @gordonwoodhull

We need both of the statements to catch all potential user routes. Removing your statement Gordon throws the "Monaco not defined" error.

I have raised a new PR.

[gordonwoodhull]

Ah, that's very interesting about the duplicated output, thanks for pointing that out.

Unfortunately your PR doesn't fix it for me. I removed the extra require because it is redundant. It isn't correct to leave a require hanging like that; one must include the code that needs the required component, inside of the callback for the require.

Note that this only happens the very first time you load the page. After that, Monaco is all loaded so there are no issues. For example, I can close the dialog and click the same button and it's fine. Are you seeing the same thing?

[gordonwoodhull]

We weren't seeing this problem before because we got the reference error #2634 instead.

[gordonwoodhull]

I think we should be able to fix this in a very stupid way: just load Monaco on initialization of the page, but asynchronously. It's incorrect and inefficient but should be good enough for 2.1. I'll take this from here.

[jameesy]

https://www.dropbox.com/s/zjdqe4nor92tr36/rcloud.mov?dl=0

I made a quick screen grab of what I am experiencing @gordonwoodhull and the behaviour of with the extra require and without.

[gordonwoodhull]

Thanks @JABedford, good to see the extra require fixes it for you. I'll try it again later.

[gordonwoodhull]

Since it's a race condition, it will depend on timing. I'm currently testing on AWS and probably have more latency than if I were testing locally. I just refreshed about 20 times and saw the fix work once.

I attempted to synchronize the load earlier but didn't get anywhere. Seems like really it should be loaded by notebook_merge instead of merger_view.

I really just need to spend a little time on it rather than trying to rush. This will push back RCloud 2.1 by a couple days but this bug is too annoying to ship.

[jameesy]

That makes sense as to why it is inconsistent.

I have placed the require in the notebook_merger file before the merge button is returned. It works for me @gordonwoodhull but would be interesting to see if it still gives you the error?

I can submit a PR with the changes if you like?

[gordonwoodhull]

I'll give it a try, but like I said before, it's only guaranteed to synchronize if everything is delayed until the require callback is called. And I found that difficult to implement.

You can see my last attempt on the synchronize-monaco-require branch. It actually fixed this bug but then the second time you open the merge dialog, the editors are replaced by little tiny squares!! Hahaha.

I've run out of time for today - I have an important demo this afternoon. Hope to try some more stuff tomo or Wednesday.

[jameesy]

Another possible option is to just open the merge dialogue and show the relevant notebook ID in the field, letting the user continue (or quit the dialogue)? Surely this will prevent the issues we are having whilst still allowing merging direct from the tree and buys us some time to investigate properly before 2.2?

[gordonwoodhull]

Thanks but no, we want the button to be easier and quicker than the dialog.

I'm fixing this by just loading Monaco at the start. It's 2MB of extra dependencies that people don't need if they don't use Merge, but I'm loading it asynchronously, so it shouldn't affect first load time.

I'll open another issue to restore "buy what you need"