Register component suites by importing.

[T4rk1n]

Refactored the component suites to register without crawling the layout by using a ComponentRegistry metaclass.

Closes #424, Closes plotly/dash-renderer#46

[T4rk1n]

@rmarren1 I don't know what the matter with the component in dash_flow_example and dash_dangerously_set_inner_html. I think they don't have a generated python Component class ?

[chriddyp]

Interesting, ComponentRegistry is like a global, shared import? This is a very cool solution

[T4rk1n]

@chriddyp It's a metaclass, __new__ get called every time a new subclass of Component get created and add the namespace to the class instance registry so it's like a global but it's not.

[chriddyp]

The other way of solving this would be by requesting components on-the-fly during rendering. That is, in here: https://github.com/plotly/dash-renderer/blob/b1cfc996563bd0b57f9487b15212dd5b782e5b3a/src/TreeContainer.js#L73
We add something like:

if (!window[component.namespace]) {
   await fetch(`/_dash-component-registry?library=${component.namespace}`).then(
      res => {
          const resources = res.scripts_and_stylesheets;
          return Promise.all(resourcesmap(lib => fetch(`/_dash-component-suites/${lib}`))
      }
   )
}

which might make it easier for other dash in other languages to do this (languages that don't have the benefit of metaclasses).

But for now, I'm 👍 with this solution as it's much simpler

[chriddyp]

This is also one of the gnarliest bugs that users in encounter, so 💯 💯 💯 for taking this on 🥇

[T4rk1n]

I first thought to fix it in the renderer but it just doesn't work like that. While you have the namespace and the component name, you don't have the actual bundle name that need to be loaded and the dist that comes with the component library.

While metaclass is a python thing, the pattern can be replicated without it. You could have the registry as a class instance/static on the base component and add the namespace to it in the constructor. I fixed the problem with the dynamically loaded components by just adding the namespace to the registry in load_components so there's no metaclass magic at play here.

[chriddyp]

you don't have the actual bundle name that need to be loaded and the dist that comes with the component library.

Right, we'd need a new endpoint for discovery (as above /_dash-component-registry?library=${component.namespace})

But in any case, this looks like a great solution 👍

[rmarren1]

@T4rk1n Yeah those repos don't have the auto-generated classes

[T4rk1n]

@rmarren1 I register those namespace in the load_component, works good.

[T4rk1n]

@plotly/dash ready for reviews.

[rmarren1]

Logic looks good, I made some suggestions for readability. Also I'm not sure if the name ComponentRegistry makes sense, maybe ComponentMeta? I think of metaclasses as the class of a class, so it sounds strange to say "The class of Component is ComponentRegistry", but ComponentMeta sounds reasonable. Also, we might want to add more functionality here in the future that would make the name ComponentRegistry stop making sense.

[rmarren1]

Suggested change

	cached = mcs.__dist_cache.get(resource_name)
	cached = mcs.__dist_cache.get(resource_name, {})

[rmarren1]

Suggested change

	__dist_cache = collections.defaultdict(dict)
	__dist_cache = {}

[rmarren1]

The default to {} is only used once, I'd rather the simpler .get(..., {}) on that one statement so it is more readable.

[rmarren1]

Suggested change

	if cached and current_len == cached.get('len'):
	if mcs.registry == cached.get('cached_module_names', set()):

[rmarren1]

Suggested change

current_len = len(mcs.registry)

[rmarren1]

Would these following three changes work? If I'm understanding correctly, we want to only return the cached.get('resources') if all the modules in the registry are also in the cache. If that is the case then I think this is more explicit.

[rmarren1]

Suggested change

	mcs.__dist_cache[resource_name]['len'] = current_len
	mcs.__dist_cache[resource_name]['cached_module_names'] = mcs.registry

[rmarren1]

Suggested change

	self._cache = {
	self._resources_cache = []

[rmarren1]

I'm not sure if there is some extra logic with the len parameter that I am not understanding, but should this work?

[T4rk1n]

The len logic is for hot-reload, it will add and remove files from the resources when changes are detected in the assets folder, not sure why I added them now and if it should be more deep.

[rmarren1]

Suggested change

	if self._cache['resources'] and cur_len == self._cache['len']:
	if len(self._resources) == len(self._resources_cache):

[rmarren1]

Suggested change

	return self._cache['resources']
	return self._resources_cache

[rmarren1]

Suggested change

self._cache['len'] = cur_len

[rmarren1]

Suggested change

	self._cache['resources'] = res = \
	self._resources_cache = res = \

[T4rk1n]

@rmarren1 Thanks for the review. I'll incorporate some of the suggestions but I fear using the gh suggestion will make a bunch of small commits that might not fit together so I'll do them together in a real commit.

"The class of Component is ComponentRegistry"

A metaclass is the type of a class.

>>> type(Component)
<class 'abc.ABCMeta'>

Also I'm not sure if the name ComponentRegistry makes sense, maybe ComponentMeta

I called it ComponentMeta first, then renamed to ComponentRegistry to reflect it's purpose. I'll go further and keep the ComponentRegistry class and
refactor the metaclass part to ComponentMeta again as it should be the type of the Component class.

[T4rk1n]

@rmarren1 I have incorporated the changes and removed the len cache busting, will integrate with hot-reload when the time come. Please review.

[chriddyp]

Once we merge this, we'll want to remove some of the references to this issue in our docs: https://github.com/plotly/dash-docs/search?q=https%3A%2F%2Fgithub.com%2Fplotly%2Fdash-renderer%2Fissues%2F46&unscoped_q=https%3A%2F%2Fgithub.com%2Fplotly%2Fdash-renderer%2Fissues%2F46 🥇

[rmarren1]

💃 Great!