Skip to content

[next] Provide less magic around the display#1673

Merged
WebReflection merged 1 commit intopyscript:nextfrom
WebReflection:better-display-with-target
Sep 5, 2023
Merged

[next] Provide less magic around the display#1673
WebReflection merged 1 commit intopyscript:nextfrom
WebReflection:better-display-with-target

Conversation

@WebReflection
Copy link
Copy Markdown
Contributor

@WebReflection WebReflection commented Sep 5, 2023

Description

This MR addresses this discussion concerns and provide n alternative that (imho) seems to be reasonably good enough, combining the minimum amount of "magic" and @hoodmane suggestions.

Changes

  • updated polyscript after fixing an issue with worker hooks and unintended indentation in scripts / tags
  • updated polyscript again to provide a way to retrieve a lazy target id whenever the worker uses display
  • removed the display as a getter from the Python module
  • avoid polluting the global context in the JS side of the equation (use an explicit JS module instead)
  • exposed a current_target() method that returns whatever current_target is on the main that is executing the code
    • this allows users to trap, if needed, such target (it's an id) as they need/please
    • this allows single py-script common use case to always work without issues, as the current_target will always be the same script/tag
    • added a test with all cases to verify the current behavior with or without a target specified and with or without an explicit import

Checklist

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

Copy link
Copy Markdown
Member

@ntoll ntoll left a comment

Choose a reason for hiding this comment

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

This looks a LOT simpler, is easier to understand and is a smaller change (while fulfilling the outcome of the referenced discussion). Thank you. 👍

I see there's outstanding conflicts that need resolving before we can merge.

@WebReflection
Copy link
Copy Markdown
Contributor Author

@ntoll thanks for the review and I am happy you like it ... I think I like it too as it's way easier to reason about what's going on and yet PyScript classic code and use cases with explicit target are still OK as they are and the most common case, a single <py-script> on the page, just got easier to explain and work with.

I have a stretch goal in mind though: provide a way to reference the main thread running script/tag via workers, so that beside PyWorker (there for feature-parity sake with JS only bootstraps but I don't see it widely used) and sync (available inevitably only in worker context) the display and the current_target would be aligned no matter where the code runs ... what do you think?

@ntoll
Copy link
Copy Markdown
Member

ntoll commented Sep 5, 2023

@WebReflection my intuition is that if we can provide a way for folks never to worry about from where they're running their code (be that in the main thread or a worker), and we do so transparently and consistently, then we're winning.

If we don't do this, many folks will bodge together a half-finished very bespoke version of what we could build as a generalised capability (and by building such a thing, we are thus saving many folks a lot of time and effort to work out how to make it work).

I imagine others will have thoughts too... @hoodmane @fpliger @antocuni @tedpatrick

Copy link
Copy Markdown
Contributor

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

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

Looks much better to me, thanks @WebReflection!

@WebReflection
Copy link
Copy Markdown
Contributor Author

right ... a couple of changes were needed to provide better worker support for non script tags, but it's not propagating so I need to wait and try later ... otherwise the test passes and everything should be good as demoed.

@WebReflection
Copy link
Copy Markdown
Contributor Author

Yup, we're good here. Unless anyone is against this current state and demo results, I'm going to merge it so that display is done.

@WebReflection WebReflection merged commit cabb1c7 into pyscript:next Sep 5, 2023
@ntoll
Copy link
Copy Markdown
Member

ntoll commented Sep 5, 2023

🎉

@JeffersGlass
Copy link
Copy Markdown
Contributor

Apologies that I missed the chance to jump in on the original conversation, but I echo all that was said - definitely appreciate the reduced magic of this version, and think it will be simpler and easier for users to understand. Thanks Andrea!

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