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

Register Context Commands with already added types #399

Merged
merged 3 commits into from
Nov 11, 2020

Conversation

mnowacki-b
Copy link
Contributor

@mnowacki-b mnowacki-b commented Nov 5, 2020

References

Issue: https://github.com/krassowski/jupyterlab-lsp/issues/398

Code changes

Registering Context Commands Manager with already added types.

User-facing changes

None

Backwards-incompatible changes

Backword compatible.

Chores

  • linted
  • tested
  • documented
  • changelog entry

@bollwyvl
Copy link
Collaborator

bollwyvl commented Nov 5, 2020

Thanks for wading into this! That constructor is getting to be a bruiser (which is not your fault, at all). I wonder if we should start wrapping some of these long constructors into IOptions kinds of patterns. There's just a lot going on! I'll comment on the code directly...

@@ -166,9 +168,7 @@ export class LSPExtension implements ILSPExtension {
console.error(reason.message);
});

adapterManager.registerExtension(this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the ordering of this line and the connect below doesn't matter, perhaps we could:

  • connect first
  • resigterExtension
    • have registerExtension (which I assume can only be called once?) do this.types.foreach((t) => this.adapterTypeAdded.emit(t))

Copy link
Member

Choose a reason for hiding this comment

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

I like this idea! In addition, it is worth noting that registerExtension already does two loops (one to connect to already registered adapters and second to watch for future registrations), so it seems like a natural place to address this problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you both for your quick response!
It sounds like a good idea; but I do have concerns (maybe they are unfounded).
If we do reemit adapterTypeAdded signal won't we cause potential problems if another extension decides to connect to it and then starts to receive multiple events with the same type?
Also, can we really assume that resigterExtension can be called only once? If so should we have something in place to enforce that?
Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bollwyvl @krassowski
I am more then happy to make the requested changes; but I thought I will make a small proposal first :)
I made some alterations please have a look and let me know what you think.
Thank you!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gah, lost what i was typing. So it goes.

So: it looks like registerExtension is expecting an LSPExtension, rather than an ILSPExtension. I think this kinda signals that it's meant for in-house usage, or would be entirely replaced, rather than having multiple. Perhaps @krassowski can suggest some tactical docstrings. If it can be multiple, then registerAdapterType probably needs to be added to the interface...

Copy link
Member

Choose a reason for hiding this comment

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

it looks like registerExtension is expecting an LSPExtension, rather than an ILSPExtension. I think this kinda signals that it's meant for in-house usage

yes, this is an accurate description.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Nov 6, 2020

Checking out locally... may I recommend a merge/rebase of master, as we bumped a lot of deps and applied a lot of formatting on #381?

...type.context_menu
});
this.feature_manager.registerCommandManager(command_manger);
registerAdapterType(
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 to extraction to a method.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Nov 6, 2020

Hooray green. So... how might we test this?

We don't expect folks to wade into non-trivial robotframework stuff, but I guess we can have a simple python function that rewrites index.out.js. I don't know off-hand what other extensions this might conflict with... have any in mind?

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thank you @mnowacki-b, this looks great to me!

@bollwyvl is this ok with you too?

@krassowski
Copy link
Member

krassowski commented Nov 6, 2020

@bollwyvl re-testing - IMO this is very much on the integration-level testing level so I am happy to accept as-is on the JS side.

@mnowacki-b did you encounter this when looking through the code or was it an interface glitch that led you to discover the race? If the latter then we might be able to create a GUI (robotframework) test for it (though testing race conditions can be tricky).

Of course, the title of the issue tells us that the context menu does not always get initialized.

@mnowacki-b
Copy link
Contributor Author

Thank you both for the quick turnaround!

@krassowski It was an interface glitch. We have a unique way of loading new extensions in our flavor of JupyterLab. We enable them during runtime based on configuration (during JupyterLab initialization). And so those extensions activation is slightly delayed.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Nov 6, 2020

our flavor of JupyterLab

We're very interested in co-developing a stable baseline for an ecosystem around language server features in Lab, but it's pretty tough, as it seems folks are just taking the code and running, and making stuff that isn't compatible with this work... which is totally cool, and why it's open source!

enable them during runtime

Can you show a tiny example that demonstrates this behavior? If your hacks are occurring too far down the stack (e.g @lumino/application though... we might not be able to support it.

@mnowacki-b
Copy link
Contributor Author

By 'our flavor' I meant a custom list of extensions. We do not modify the core JLab we only specify a list of custom extensions that we are interested in. Most of the extensions we use are open sourced with a handful of internal extensions.

I still believe this is an issue that may occur as the plugins are loaded asynchronously and the list of adapter type can be updated before registerExtension occurs.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Nov 6, 2020

the list of adapter type can be updated before registerExtension occurs.

Perhaps another signal/PromiseDelegate is needed? A thing we resort sometimes for these topics is ASCII art sequence diagrams, drawio pictures, etc.

More broadly: I hope I'm not coming off as belligerent: we're at a disadvantage here, as we haven't seen this (mis)behavior in action. If we can work together to reproduce and test it, in a real browser, we can make sure it keeps working. It may mean this is a "custom" lab extension/excursion in CI, but it's work worth doing.

Our semi-ridiculous integration suite has been the only we have been able to confidently add new features and extension points, and handle migrations between different JupyterLabs... and things we don't test have definitely regressed.

This kind of testing will be much better with the forthcoming jupyterlab 3 work being discussed on #400, where flipping the installed extensions will be measured in milliseconds, not minutes, but I can totally understand if you've got commitments to lab2 (i still ship lab 1 on some projects, because some in-house extensions never updated).

@mnowacki-b
Copy link
Contributor Author

Totally understand, I can try to see if I can come up with something that can reproduce the issue.
We are actually looking forward to migrating to 3.0 as well; however, we have to wait a little bit before we can do that.
We are also very excited about adding juputerlab-lsp and we just don't want to wait :).

@mnowacki-b
Copy link
Contributor Author

mnowacki-b commented Nov 7, 2020

I put together this rough diagram to demonstrate how this race condition can occur.
As you can see in this scenario all it takes is for INotebookTracker to initialize before all of the ILSPFeatureManager dependencies are initialized. This will result in Notebook's context menu without LSP menu items.

lsp-race

ILSPAdapterManager:
   requires: [ILabShell]
   - emits signal when new adapter type is added `adapterTypeAdded.emit`

NOTEBOOK_ADAPTER (@krassowski/jupyterlab-lsp:NotebookAdapter):
   requires: [ILSPAdapterManager, INotebookTracker]
   - on activation adds notebook adapter type to `ILSPAdapterManager`

ILSPFeatureManager: 
   requires: [ISettingRegistry, ICommandPalette, IDocumentManager, IPaths, IStatusBar, ILSPAdapterManager, ILSPVirtualEditorManager, ILSPCodeExtractorsManager, ILSPCodeOverridesManager]
   - instantiates LSPExtension which currently in its constructor calls `adapterManager.registerExtension` and `adapterManager.adapterTypeAdded.connect`

@mnowacki-b
Copy link
Contributor Author

So another aproceh to fix this would be require NOTEBOOK_ADAPTER and FILE_EDITOR_ADAPTER to wait for ILSPFeatureManager

@mnowacki-b
Copy link
Contributor Author

@mnowacki-b
Copy link
Contributor Author

Is there anything I can do to expedite this PR?

@krassowski
Copy link
Member

I think that we can merge it as is. I think the current approach is a step forward and even if we do not test against the faulty behaviour, the changes improve the quality of code. Thank you for all the help.

@krassowski krassowski merged commit 192d016 into jupyter-lsp:master Nov 11, 2020
@mnowacki-b mnowacki-b deleted the context-menu branch November 11, 2020 21:08
@mnowacki-b
Copy link
Contributor Author

mnowacki-b commented Nov 11, 2020

Thank you!
I am still fairly new to open source contributions; if there is anything I can help with please let me know.

If I may ask; what it your deployment process? how often do you cut releases?

@krassowski
Copy link
Member

I will probably do one once #406 is in. Sorry extremely busy week for me.

@mnowacki-b
Copy link
Contributor Author

@krassowski Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants