Skip to content

Reconsider how we use/advertise idom.run() #657

Description

@Archmonger

Current Situation

idom.run() probably shouldn't be the primary advertised way of using IDOM. Especially in it's current state.

Using a development webserver for production use is a big no-no, and by prominently displaying idom.run() as the first steps to using IDOM shows we are encouraging people to do just that. I believe the primary way of utilizing IDOM should be embedding within existing applications.

Proposed Actions

  • Keep idom.run() as is, but add logging to indicate idom.run() is not suited for production use.
    • ex. idom.run() is only intended for testing purposes. For production use, consider embedding IDOM within an existing project. Check out the docs for more info.
  • Expand the ease-of-use for integrating into supported web frameworks, similar to Django-IDOM
  • Remove SharedClientState, as it is not compatible with multiprocessed webservers.
    • This is also an opportunity to rename PerClientState to IdomView

Optional

  • Change the callable name/path to more appropriately indicate danger or for testing purposes only
    • idom.testing.webserver()
    • idom.shortcuts.run()
    • idom.devtools.run_webserver()
  • Remove the pip parameter for frameworks (it's really not needed)
    • idom.run() should raise a simple to understand exception if the user has forgotten to install a web framework
    • ex. Looks like you don't have a web framework installed. You'll need to pip install one of the following: fastapi, flask, sanic

Activity

  1. Archmonger commented on Feb 8, 2022

    @Archmonger
    ContributorAuthor

    My personal opinion is that idom.run() should either be removed or tucked away into a "dev tools" closet as-is and shouldn't be prominently advertised.

    Instead, we should focus on simplifying how users integrate IDOM into existing applications. It's an over-reach of scope for us to handle anything related to launching a WSGI/ASGI webserver.

  2. added
    priority-2-moderateShould be resolved on a reasonable timeline.
    priority-3-lowMay be resolved one any timeline.
    type-revisionAbout a change in functionality or behavior
    on Feb 8, 2022
  3. rmorshea commented on Feb 8, 2022

    @rmorshea
    Collaborator

    We could add a big fat warning about this instead? Technically doing idom.run is fine with Sanic (and I think Tornado too), but not the others.

  4. rmorshea commented on Feb 8, 2022

    @rmorshea
    Collaborator

    Meh, nevermind. We should completely shift how this works:

    • Drop the run function
    • stop calling things a per-client state "server" and instead call it a "view"
    • Make app a required argument of PerClientStateView etc.
    • Rename idom.server.proto.ServerType.run to run_dev_server

    Doing this with appropriate deprecation warnings will be a bit tricky, but I think that sends a more accurate message about how you should expect to use IDOM.

  5. rmorshea commented on Feb 8, 2022

    @rmorshea
    Collaborator

    The only question then is how you many the "hello world" still look simple...

  6. rmorshea commented on Feb 8, 2022

    @rmorshea
    Collaborator

    Maybe do:

    from idom import component, html, server
    
    
    @component
    def App():
        return html.h1("Hello, World!")
    
    
    server.default.run(App)

    Except run only accepts one argument (use lack of configuration as deterrent) and produces a big fat warning.

    or maybe server.dev.run?

  7. rmorshea commented on Feb 8, 2022

    @rmorshea
    Collaborator

    There's also a question of the install target: #600

    I think we should we remove any sense of a "default" install target too since you'd always need to explicitly call out which server implementation you're using.

    There's no way to give a deprecation warning for this removal though as far as I can tell unless pip knows to find the last version with a given install target.

  8. Archmonger commented on Feb 8, 2022

    @Archmonger
    ContributorAuthor

    Doing this with appropriate deprecation warnings will be a bit tricky, but I think that sends a more accurate message about how you should expect to use IDOM.

    IDOM is still <1.0.0, technically we've never needed to deprecate anything. Immediate breaking changes are fully acceptable on pre-release version numbers.

    The only question then is how you many the "hello world" still look simple...

    Maybe we consider a pattern like this (with the run callable name being TBD)

    from idom import component, html, devtools
    from fastapi import FastAPI
    
    fastapi_app = FastAPI()
    
    @component
    def Hello():
        return html.h1("Hello, World!")
    
    # This should print a warning that it is not production usable
    devtools.run(fastapi_app, component=Hello)
  9. rmorshea commented on Feb 8, 2022

    @rmorshea
    Collaborator

    I'm worried about the hello-world implying a preference for a given server implementation.

  10. Archmonger commented on Feb 8, 2022

    @Archmonger
    ContributorAuthor

    That's pretty much an impossible problem to solve without multiple examples for each framework. We support multiple web frameworks, but we need to choose at least one to first demo with.

    Could just be a comment saying "Utilize whatever Python web framework you like here!"

  11. 6 remaining items

  12. Archmonger commented on Feb 8, 2022

    @Archmonger
    ContributorAuthor

    I really dislike the name PerClientState

    I've gotten an inverse comment at some point. With that said, I had a hard time coming up with a name that accurately described what they did so I'd be open to suggestions.

    Just removing State from the name makes it a lot easier to understand. For example PerClientView and SharedView.

    On the topic of SharedView

    The multiprocessing issue with SharedView will become an even bigger problem as we start recommending people to use actual webservers to serve IDOM. I'm sure the majority of people use multiple worker processes in any realistic deployment scenario.

    I'd honestly just recommend removing it now. As far as I know, it's not possible to obtain a multiprocessing safe SharedView. Especially in the context of a webserver's ASGI worker processes, since we don't have direct access to the worker process callable.

    Here's what I think all our options are

    • Remove SharedView and have shared states to be left up to the user
    • Remove SharedView and create framework-specific shared state hooks when technologically possible
    • Have redis become a requirement for SharedView

    If we completely remove SharedView, then we also gain the luxury of renaming PerClientView to something simple like IdomView.

  13. Archmonger commented on Feb 8, 2022

    @Archmonger
    ContributorAuthor

    On a side note, I've decided to commit to creating IDOM video tutorials. We're going to need something for our visual learners to increase adoption.

    I'll try and make that happen sometime this year.

  14. Archmonger commented on Feb 9, 2022

    @Archmonger
    ContributorAuthor

    Let me know if you agree on #657 (comment)

  15. added
    priority-3-lowMay be resolved one any timeline.
    and removed
    priority-2-moderateShould be resolved on a reasonable timeline.
    on Feb 9, 2022
  16. added this to the 1.0 milestone on Feb 16, 2022
  17. rmorshea commented on Feb 17, 2022

    @rmorshea
    Collaborator

    I think this issue may be related: #653

  18. rmorshea commented on Feb 17, 2022

    @rmorshea
    Collaborator

    Ah, whoops, didn't read through your comment.

    Yes, I think I agree.

  19. rmorshea commented on Feb 17, 2022

    @rmorshea
    Collaborator

    Probably with the addition that idom.run takes a single argument (the component) to discourage use in the long term.

  20. Archmonger commented on Feb 17, 2022

    @Archmonger
    ContributorAuthor

    How do you feel about removing SharedView? #657 (comment)

  21. rmorshea commented on Feb 17, 2022

    @rmorshea
    Collaborator

    I agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    priority-3-lowMay be resolved one any timeline.type-docsAbout changes and updates to documentationtype-revisionAbout a change in functionality or behavior

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions