8000 The all-new, pyscript.web (ignore the branch name :) ) by mchilvers · Pull Request #2129 · pyscript/pyscript · GitHub
[go: up one dir, main page]
Skip to content

The all-new, pyscript.web (ignore the branch name :) ) #2129

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 39 commits into from
Aug 1, 2024
Merged

The all-new, pyscript.web (ignore the branch name :) ) #2129

merged 39 commits into from
Aug 1, 2024

Conversation

mchilvers
Copy link
Contributor

Description

Minor polishing.

Changes

  • Gather all Element classes at the bottom of the module.
  • other minor comments/cleanups

Checklist

  • All tests pass locally
  • I have updated CHANGELOG.md
  • I have created documentation for this(if applicable)

@WebReflection
Copy link
Contributor
WebReflection commented Jul 25, 2024 •
edited
Loading

In general this looks good to me but I have some issue around self._parent dance ... let me explain ...

I understand the moment it's accessed as property, if the element has a parentElement (also note that parentElement is different from parentNode) then it gets assigned so that next time it won't be needed to re-calculate it.

However, that is not how highly dynamic DOM works so that if I accessed a node.parent once and that node gets moved somewhere else after, the parent will still return the previous parent element.

Also if a parent element empties itself, that node with _parent would still point at something even if the element is not live.

I think self._parent = ... should be a guard to avoid repeated logic but every time the node.parent is accessed if it had a value you need to check that in fact its _dom_element.parentElement is the same as the previous self._parent._dom_element and, if that's not the case, you should re-assign it or remove it if the element is not live yet.

A lot of "live functionality" can indeed lead to confusion if we cache too much and while I fully embrace and promote caching whenever it's possible, it looks to me like a WeakMap to relate DOM nodes identities to their counter Python wrapper is missing around some logic but I have no idea if Python has WeakMap concept anywhere in its standard lib, considering also both MicroPython and Pyodide cross-compatibility.

edit P.S. @mchilvers I know _parent is not even part of this MR but to read the whole file without too much diffing noise around I have spotted that issue and maybe this is a good opportunity to fix that too?

@mchilvers
Copy link
Contributor Author

@WebReflection I agree. The cached _parent thing has always smelled to me. My preference would be don't cache/guard it at all for now - we can always try something if performance is actually an issue.

mchilvers and others added 23 commits July 26, 2024 09:59
@mchilvers
Don't cache the element's parent.
035e2e1
@mchilvers
Remove style type check until we decide whether or not to add for cla…
3714711 
…sses too.
@mchilvers
Add ability to register/unregister element classes.
7d6f1e9
@mchilvers
Implement __iter__ for container elements.
9e6f65a
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
e2c0b88 
for more information, see https://pre-commit.ci
@mchilvers
Minor renaming to make it clear when we have an Element instance vs a…
8110fd7 
…n actual DOM element.
@mchilvers
Merge branch 'mc/pyscript-web-minor-cleanup' of github.com:pyscript/p…
bef0a90 
…yscript into mc/pyscript-web-minor-cleanup
@mchilvers
remove duplication: added Element.get_tag_name
7a066ec
@mchilvers
Commenting.
24c1e37
@mchilvers
Allow Element.append to 1) use *args, 2) accept iterables
ab1178f
@mchilvers
Remove iterable check - inteferes with js proxies.
52779ba
@mchilvers
Don't use *args, so it quacks more like a list ;)
73ab702
@mchilvers
Element.append take 2 :)
395c785
@mchilvers
Remove unused code.
3725f0c
@mchilvers
Move to web.py with a page object!
fe7f70f
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
bbee95a 
for more information, see https://pre-commit.ci
@mchilvers
Added 'page.title' too :)
6e02a84
@mchilvers
Merge branch 'mc/pyscript-web-minor-cleanup' of github.com:pyscript/p…
38d147b 
…yscript into mc/pyscript-web-minor-cleanup
@mchilvers
Add __getitem__ as a shortcut for page.find
ef94e29
@mchilvers
Add Element.__getitem__ to be consistent
b9b239b
@mchilvers
Make __getitem__ consistent for Page, Element and ElementCollection.
ef4a185
@mchilvers
Docstringing.
0d9b1fb
@mchilvers
Docstringing.
1eed03a
@mchilvers mchilvers changed the title Minor cleanups: move all Element classes to bottom of module. The all-new, pyscript.web (ignore the branch name :) ) Jul 31, 2024
@ntoll ntoll merged commit 999897d into main Aug 1, 2024
2 checks passed
@ntoll ntoll deleted the mc/pyscript-web-minor-cleanup branch August 1, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntoll ntoll ntoll approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mchilvers @WebReflection @ntoll
0