Use import_names and component_names to error on dependency.py conflicts
#5495
Conversation
|
And if we unique-key by name, instead of unique-key by hash-based key, we get maximum savings in the following image (hence why I thought of this when working on #5493: 
|
|
No I think we should test this, in retrospect. |
@evnchn Are you planning to write tests for this PR? To be honest, I'm not sure how to do it without too much effort... Another question: Should we cross-check import and component names? # In register_vue_component (lines 91 and 100):
assert name not in component_names, f'Component name "{name}" is already used'
assert name not in import_names, f'Component name "{name}" conflicts with library/ESM module name'
# In register_library (line 115):
assert name not in import_names, f'Library / ESM module name "{name}" is already used'
assert name not in component_names, f'Library / ESM module name "{name}" conflicts with component name'
# In register_esm (line 141):
assert name not in import_names, f'Library / ESM module name "{name}" is already used'
assert name not in component_names, f'ESM module name "{name}" conflicts with component name' |
Ideally we should, but 3.4.1/3.5 is round the table, and I am kinda busy until 20th, so I am not sure. Maybe we can skip that for now?
Actually yes. Consider:
Collision in name is lethal. So we should error out in a cross-checked manner. But I also have an interesting thought: Rather than erroring out, can NiceGUI dodge conflicts when they arise? Seeing that we already imported components under a different name in #5562 ( But how much do we want to endorse this? I feel like we shouldn't dodge for the user, if (1) it is bad behaviour in the first place, (2) right now the code doesn't work so why make it work and expand the API capabilities. |
|
While code is clean, the perf may not be good if the set is recreated every time the check occurs. The old approach of maintaining a set may be still good, if we pack that into |
|
I inspected the set we are testing against: def _name_is_unique(name: str) -> bool:
esm_names = {esm_module.name for esm_module in esm_modules.values()}
import os
os.system('clear')
print({'vue', 'sass', 'immutable', *vue_components, *js_components, *libraries, *esm_names})
return name not in {'vue', 'sass', 'immutable', *vue_components, *js_components, *libraries, *esm_names}And got the following results: Why are the keys showing up? We care about the names. Something isn't right... |
|
The previous commits introduced some logic issues: 3badc76 fixes how keys are found in a set which should contain only names. This induces false-positive as InteractiveImage and InteractiveImageLayer shares component. Under existing architecture, it should allow to return the existing component if key and path match for the existing entry. Hence |
|
Original implementation of Naive implementation of seen_names: set[str] = {'vue', 'sass', 'immutable'}
def _name_is_unique(name: str) -> bool:
if name in seen_names:
return False
seen_names.add(name)
return TrueTest code: time_start = time.time()
for _ in range(100000):
_name_is_unique('blah')
print(f'Tested _name_is_unique 10000 times in {time.time() - time_start} seconds')@falkoschindler Sorry but I would prefer going back. What do you think? |
|
Finally: If we need more time then maybe we ship 3.4.1 first; If not we can slide this right in. |
|
Sure, the performance is better using a set of The confusion between keys and names is definitely something I messed up. This is an advantage of keeping a plain set of names. Keeping I'd love to keep the Bottom line: Let's leave the code as it is. Performance doesn't really matter, but architecture is simpler this way. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Oh dear, apparently this PR breaks user code. Consider this line: class SignaturePad(ui.element, component='signature_pad.js', esm={'signature_pad': 'dist'}):It's certainly debatable whether the ESM module should have the same name like the Vue component. But it used to be possible, so we shouldn't break it. And if it was possible, our uniqueness constraint might be wrong. |
|
Since this PR isn't urgent at all, I'll postpone it to 3.5. No need to rush. |
|
Yeah basically this PR should precisely capture the original behavior's boundary and throw and error, no more no less. Right now, we have overdone it. Let's fix later. |
|
[With some help from AI...] SummaryThe PR enforced a single global namespace for all names, but component names and ESM module names are in different namespaces:
These don't conflict, so they can share the same name. The FixSeparated name tracking into two namespaces:
Created two functions:
Now:
This restores backward compatibility for cases like |
|
So actually cross-checking was not quite right, and we are back at #5495 (comment) My response in #5495 (comment) was a bit confused. Sorry. |
|
No worries. I was confused as well. In b8f54a1 I experimented with moving all assertions into the dataclasses:
|
evnchn
left a comment
evnchn
left a comment
There was a problem hiding this comment.
Spotted a logic issue
falkoschindler
added
feature
… we drop it (#5500) ### Motivation While working on #5495 (comment), I realize there's literally space savings on-the-table, by inspecting code behaviour. ### Implementation #### Observation: Only 2 places where JS dependencies are loaded 1. In the HTML, we add JS imports statically https://github.com/zauberzeug/nicegui/blob/eea9a48071f274657e65409571c255b9b35830e8/nicegui/dependencies.py#L193-L206 2. In JS, we load them https://github.com/zauberzeug/nicegui/blob/eea9a48071f274657e65409571c255b9b35830e8/nicegui/static/nicegui.js#L281-L290 There is no other place. I looked. #### Fulfill the bare-minimum for the code logic For 1 to not trigger 2, we do the following: https://github.com/zauberzeug/nicegui/blob/eea9a48071f274657e65409571c255b9b35830e8/nicegui/static/nicegui.js#L148-L149 But note we only care about the `.name`, nothing else. So why ship the bulk? ### Progress - [x] I chose a meaningful title that completes the sentence: "If applied, this PR will..." - [x] The implementation is complete. - [x] Pytests are not necessary. - [x] Does existing tests pass? - [x] Documentation is not necessary (identical behaviour but faster). ### Final note This re-affirms the belief in #5495 that we key by name, not by hash-key. **Is this a simple-win? I think it is if you memorize the codebase.** --------- Co-authored-by: Falko Schindler <[email protected]>
2 participants
Motivation
TL-DR:
dependency.pyisn't coded as tightly as it should, silently letting (possibly critical) errors slide into client-side instead of stopping them right away and preventing the server from launching in a verbose and attention-grabbing manner.I was working on #5493 and looking for space savings, when I found some interesting behaviour with regards to our dependency.py implementation.
importsat 2 places, libraries and ESM modulesnicegui/nicegui/dependencies.py
Lines 180 to 186 in eea9a48
If ESM modules are to have the same name as a library, the library would be shadowed.
imports intojs_importsat 2 places, one for.jscomponents, one for.vuecomponentsnicegui/nicegui/dependencies.py
Lines 193 to 207 in eea9a48
This one is a bit more serious: If you run import twice with same name, browser JS errors out entirely, and the page grinds to a halt (white screen)
Uncaught SyntaxError: Identifier 'BLAH' has already been declaredImplementation
import_namesandcomponent_namesProgress
Final notes
May break existing code which works (barely) in case 1, but I think we should break it, because the code isn't valid in the first place.