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

Added provider-consumer example (multiple related packages). #245

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

ericsnekbytes
Copy link
Contributor

@ericsnekbytes ericsnekbytes commented Sep 1, 2023

This example adds three related extension packages:

  • step_counter
  • step_counter_extension
  • leap_counter_extension

With all 3 installed in a JupyterLab environment, users will get a provider extension that counts steps (a token + a default service implementation that holds step count data), and two alternate consumer plugins that consume the provider's step_counter service and provide disparate interactivity via buttons in each extension that count steps differently.

Provider/consumer extension sample image

@ericsnekbytes ericsnekbytes changed the title Added provider-consumer example (multiple related packages). (DRAFT) Added provider-consumer example (multiple related packages). Sep 1, 2023
@ericsnekbytes ericsnekbytes requested a review from jtpio September 1, 2023 19:49
@ericsnekbytes
Copy link
Contributor Author

@jtpio This is pretty much done (minus some small formatting items and the signalling issue mentioned above) if you want to take a look. Keep in mind that this is intended to be a MINIMAL demonstration of the features needed to implement the provider-consumer pattern (I skipped over several opportunities to make it "better" in favor of highly reduced complexity).

@ericsnekbytes ericsnekbytes changed the title (DRAFT) Added provider-consumer example (multiple related packages). Added provider-consumer example (multiple related packages). Sep 29, 2023
@ericsnekbytes ericsnekbytes marked this pull request as ready for review October 5, 2023 17:23
@ericsnekbytes
Copy link
Contributor Author

@fcollonval If you have time to review I'd like to get this approved, the Dual Compatibility document has a spinoff "Plugin System" document (see top post description) that depends on this example so this should be merged first. My plan now is to try to get the document and examples merged first, then do a follow-up PR for the unit tests and repo-referencing for the code snippets.

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.

Some suggestions as promised.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved

leapButton: HTMLElement;
combinedStepCountLabel: HTMLElement;
counter: any;
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid any as an ani-pattern and because it hides the important bit that this is StepCounter

Suggested change
counter: any;
counter: StepCounter;

The three members should probably be private; it is of no importance in other examples but here distinctions between private/public might (?) be useful to highlight that these things are not to be shared with the outside ("You depend on LeapCounterWidget and want to touch the counter? Request it explicitly in requires, don't touch my counter!"

this.updateStepCountDisplay();
}

// Refresh the displayed step count
Copy link
Member

Choose a reason for hiding this comment

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

I would move the comment inside the function or otherwise use /* */ notation to teach good practices.

// Notice that the constructor for this object takes a "counter"
// argument, which is the service object associated with the StepCounter
// token (which is passed in by the consumer plugin).
constructor(counter: any) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
constructor(counter: any) {
constructor(counter: StepCounter) {

Comment on lines +6 to +8
it('should be tested', () => {
expect(1 + 1).toEqual(2);
});
Copy link
Member

Choose a reason for hiding this comment

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

Do you intend to implement the test here?

// use it to type-check any service-object associated with the
// token that a provider plugin supplies to check that it conforms
// to the interface.
interface StepCounterItem {
Copy link
Member

Choose a reason for hiding this comment

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

Oh, so I see why you used any earlier. The problem here is twofold:
a) StepCounterItem is not exported
b) It is named differently from StepCounter so we do not get the benefit of declaration merging

Typically we export just one symbol which is a merge token and interface. I see a potential for didactic value of not doing so, but then in real life we do it because it allows for a clean one symbol export.

Copy link
Contributor Author

@ericsnekbytes ericsnekbytes Nov 17, 2023

Choose a reason for hiding this comment

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

I purposefully did not use declaration merging because this is a beginner tutorial for these concepts, I think it's VERY likely to cause problems/confusion for non-veterans (again, our student or biologist, chemist, finance users who are smart but probably not experienced web developers), and as a 6-7 year desktop applications developer I can say personally that it caused me some difficulties (I found it a very peculiar and unintuitive language feature/pattern).

// https://jupyterlab.readthedocs.io/en/latest/extension/extension_dev.html#requiring-a-service
// https://jupyterlab.readthedocs.io/en/latest/extension/extension_dev.html#optionally-using-a-service

export { StepCounter, StepCounterItem };
Copy link
Member

Choose a reason for hiding this comment

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

Oh, you export both here. I guess I do not understand why you used any after all. Still let's consider using (and explaining declaration merging).

@ghost
Copy link

ghost commented Nov 17, 2023

@ericsnekbytes does this work with jupyterlab 3?

@ericsnekbytes
Copy link
Contributor Author

@ericsnekbytes does this work with jupyterlab 3?

@terlan4 It does work in JupyterLab 3, just tested it. The Provider/Consumer pattern is a foundational piece of JupyterLab's design and of its extension system, (so it is certainly relevant to both).

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.

2 participants