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

Fix dependencies between Dash components #2105

Merged
merged 4 commits into from
Nov 2, 2022

Conversation

pikhovkin
Copy link
Contributor

@pikhovkin pikhovkin commented Jun 23, 2022

I have a dash-component1 that depends on react-lib1. I also have dash-component2, which depends on react-lib1 and dash-component1. dash-component2 is a plugin to dash-component1.
Since ComponentRegistry.registry is an unordered set, the Dash components added to it may not be in the order in which they were added to it.
I need to load scripts (dash components) in the order in which they were imported.
I suggest a small change in dash/development/base_component.py

...

class OrderedSet(collections.OrderedDict):
    def add(self, item):
        self[item] = None


class ComponentRegistry:
    registry = OrderedSet()  # set() <<
...

I have a dash-component1 that depends on react-lib1. I also have dash-component2, which depends on react-lib1 and dash-component1. Dash-component2 is a plugin to dash-component1.
Since ComponentRegistry.registry is an unordered set, the dash components added to it may not be in the order in which they were added to it.
I need to load scripts (dash components) in the order in which they were imported.
@alexcjohnson
Copy link
Collaborator

Thanks @pikhovkin! Just to be sure I understand the situation, you're trying to ensure that the files in _js_dist for dash-component1 are included on the page before the files in _js_dist for dash-component2, so that dash-component2 can use items loaded by dash-component1 during script execution? This might be avoidable if the dependency were moved into component initialization instead of script execution, but that might not always be feasible.

Depending on import order feels a bit fragile to me, but I suppose you can have dash-component2 itself import dash-component1 in its __init__.py, so that this doesn't depend on the user getting the order right, is that what you intend to do? I guess if we document this as the "official" way to have one component depend on another, that would cover the case where the first package is required to use the second. There might be another use case of two packages both independently needing the same resource and you only want to load it once; perhaps we can ignore that case for now.

I think this would be better with https://pypi.org/project/ordered-set/ rather than using OrderedDict as a stand-in, just in case users are depending on ComponentRegistry.registry having set behavior that the Dash core itself doesn't use.

@pikhovkin
Copy link
Contributor Author

pikhovkin commented Jun 27, 2022

@alexcjohnson thanks for the answer, but you misunderstood.
Regardless of where and when the dependencies are imported, the order of those dependencies depends on the unordered ComponentRegistry.registry, which does not preserve the order of the elements.
Try run

>>> s = set(['h'])
>>> s.add('a')
>>> s
{'a', 'h'}
>>> s.add('g')
>>> s
{'a', 'h', 'g'}

I think this would be better with https://pypi.org/project/ordered-set/ rather than using OrderedDict as a stand-in, just in case users are depending on ComponentRegistry.registry having set behavior that the Dash core itself doesn't use.

You can use third-party libraries if you like. Although only the add method is used in the ComponentRegistry.registry. My microvariant works fine. And you can always use a third-party library later.
And also https://pypi.org/project/ordered-set uses the dict internally. Why pay more? :-)

@alexcjohnson
Copy link
Collaborator

which does not preserve the order of the elements

I understand that, I'm just trying to pin down WHY I would care about that order. Most of the time import order is irrelevant, as is the case for Python and JS imports in general. There are conventions about what order you should put imports at the top of a module, but it's supposed to be just conventions. If the behavior is different when you change that order it would generally be considered a bug. Did I understand correctly that what matters to you is the order the JS scripts are executed on the page? And that (after we make ComponentRegistry.registry preserve order) having your dash-component2 import dash-component1 internally would hide this order dependency from your users?

Although only the add method is used in the ComponentRegistry.registry

Right, that's what I meant by "behavior that the Dash core itself doesn't use." What I'm concerned about is other users of Dash that may have found a reason to inspect ComponentRegistry.registry in their own code and therefore depend on it having other set behavior. I'm not aware of any such uses, but Dash has lots of very clever users and we don't want to break their code :)

@T4rk1n
Copy link
Contributor

T4rk1n commented Jun 28, 2022

I think this would be better with https://pypi.org/project/ordered-set/ rather than using OrderedDict as a stand-in, just in case users are depending on ComponentRegistry.registry having set behavior that the Dash core itself doesn't use.

I also think it would be best if this was a proper set.

@pikhovkin
Copy link
Contributor Author

I'm just trying to pin down WHY I would care about that order

The order is important when the plugin should work. The plugin depends on the parent component (a dash-my-plugin-component depends on a dash-third-party-component).
In my case, the plugin needs to get the parent context.
When I say a "plugin" is a plugin, it wraps a react component that needs a parent context. That is why the order in which scripts are loaded is important here.
All Dash components that I have seen contain all the features in themselves, in their internal scripts. Therefore, they can be imported in any order. The script loading order for a Dash component will be provided by _js_dist.
But there are cases when you need to develop components with more complex dependencies, where the script loading order is important, where the parent component must be imported before the plugin. Here is just such a case.

I'm not aware of any such uses, but Dash has lots of very clever users and we don't want to break their code

Have you ever seen an example of someone doing something with .registry? I'm willing to bet that 99.9(9)% Dash users don't know anything about .registry. .registry is an undocumented feature. The remaining percentage of users are taking a reasonable risk by doing something with .registry other than adding an element.
But I don't mind you using a third party package to keep order in .registry.

@emilhe
Copy link
Contributor

emilhe commented Jul 22, 2022

I am also interested in progress on this PR. As I understand, it would enable people to wrap leaflet plugins as separate Dash components, and use them with dash-leaflet. Which would be awesome :)

@pikhovkin
Copy link
Contributor Author

@alexcjohnson How can I help to speed up merging of this PR?

@alexcjohnson
Copy link
Collaborator

@pikhovkin as I said before I'm not aware of any external uses of ComponentRegistry.registry, but being undocumented is different from not being public. There are no leading underscores to access it, so it's part of our public API and we should try not to break it.

That being said, there are actually three things Dash depends on this object doing (.add, in and list()), your implementation does all three correctly, and these are the main things I can imagine folks wanting to do if they were to find a use for directly inspecting the registry.

The only other thing I can think of is maybe removing items? Like if package A gets pulled in by package B, so A is implicitly added to the registry, but you don't actually want package A (and the extra JS it loads on the page). Then you could imagine wanting to remove A from the registry. Sets have discard and remove methods for this, that dicts don't have. They also have pop with a different signature from dict.pop, though I can't imagine users wanting to use this.

So we could add these methods, or even wait to see if anyone complains and then add them... but to my mind it'd be easier to just use the ordered-set package and be done with it, it's a pretty simple package so a minor cost to pay. Just make sure to allow at least down to v4.0 since Dash still supports Python 3.6 - or don't even restrict its version at all, it's been stable a long time.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Thanks @pikhovkin and @T4rk1n

@T4rk1n please add a changelog entry, then we can merge.

@T4rk1n T4rk1n merged commit 0bc0d32 into plotly:dev Nov 2, 2022
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.

4 participants