Skip to content

Conversation

wch
Copy link
Collaborator

@wch wch commented Oct 17, 2023

To install:

# First uninstall old version
pip uninstall shiny

pip install git+https://github.com/posit-dev/py-shiny.git@flat-mode#egg=shiny

Express apps can be run with:

shiny run examples/express/app.py

You can also run it with an ASGI server like uvicorn, like so:

SHINY_EXPRESS_APP_FILE="examples/express/app.py" uvicorn shiny.express.app:app

This works because shiny.express.app:app is an ASGI application, and so ASGI servers like uvicorn will be able to use it. However, it is part of the shiny package and can't be user-modified, so the the way you tell it which file to use for the flat app source is by setting SHINY_EXPRESS_APP_FILE.

@jcheng5
Copy link
Collaborator

jcheng5 commented Oct 17, 2023

I don't feel comfortable enough with the @no_default_ui decorator to push it to you branch, but here's the implementation:

def no_default_ui(output_renderer: OutputRenderer[OT]) -> OutputRenderer[OT]:
    output_renderer.default_ui = lambda x: ""
    return output_renderer

Bool-on-OutputRenderer route is equally easy.

I put it in shiny/render/transformer/_transformer.py but then made it available from shiny/render/__init__.py.

} else {
// This is a later run. Reset attributes to their inital state.
for (const attr of this.originalBodyTagAttrs) {
el.setAttribute(attr.name, attr.value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to also clear added attributes

}
}

let content = typeof data === "string" ? data : data.html;
Copy link
Collaborator

Choose a reason for hiding this comment

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

data will never be a string


// Parse the HTML
const parser = new DOMParser();
const doc = parser.parseFromString(content, "text/html");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be a little slow but whatever

app: App


def __getattr__(name: str):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overkill?

"output_args",
"suspend_display",
"wrap_express_app",
"app",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems private

return old_cm


class DetectShinyExpressVisitor(ast.NodeVisitor):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider rejecting if shiny.App() call is found

return app


def is_express_app(app: str, app_dir: str | None) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe have a way out as well--environment variable or something to force one mode or another.

# returns the input for the current session. This will work in the vast majority of
# cases, but when it fails, it will be very confusing.
def __getattr__(name: str) -> object:
if name == "input":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider detecting if input, output, or session are requested more than once from the same scope. (using inspect?)

Comment on lines 53 to 57
app_ui = ui.page_output("__page__")

def express_server(input: Inputs, output: Outputs, session: Session):
try:
dyn_ui = run_express(file)
Copy link
Collaborator

@jcheng5 jcheng5 Nov 8, 2023

Choose a reason for hiding this comment

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

Suggested change
app_ui = ui.page_output("__page__")
def express_server(input: Inputs, output: Outputs, session: Session):
try:
dyn_ui = run_express(file)
app_ui = run_express(file)
def express_server(input: Inputs, output: Outputs, session: Session):
try:
run_express(file)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reasons for suggesting this (during live discussion):

  1. HTML dependencies load with the page load which leads to smoother rendering (less fouc)

Downside:

  1. Process startup is slightly slower

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, also would need to get rid of render.ui below

file = Path(os.getcwd()) / app_file

# TODO: title and lang
app_ui = ui.page_output("__page__")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whatever we do, need to catch errors that happen during UI rendering/app.py loading and have the stack trace appear in the console.

sys.displayhook = set_result

reset_top_level_recall_context_manager()
get_top_level_recall_context_manager().__enter__()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should use with

nonlocal ui_result
ui_result = cast(Tag, x)

sys.displayhook = set_result
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably this too

reset_top_level_recall_context_manager()
get_top_level_recall_context_manager().__enter__()

file_path = str(file.resolve())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe require this to be absolute rather than resolving it here? Or not, I don't know.

for node in tree.body:
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
exec(
compile(ast.Module([node], type_ignores=[]), file_path, "exec"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

AST loading, transformation, and compiling could be a separate step that we only do once. Then separate step for running each code block, which we do every time.



_top_level_recall_context_manager: RecallContextManager[Tag]
_top_level_recall_context_manager_has_been_replaced = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

When you use a with layout.foo() function in the wrong setting, you get an unhelpful error message

return ui_result


_top_level_recall_context_manager: RecallContextManager[Tag]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider making all this state/logic an object/methods

if value is not None:
self.args.append(tags.pre(repr(value)))

def __enter__(self) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The global context manager object could accumulate the results, and then only decide to create the page at the end.


self._prev_displayhook = sys.displayhook
# Collect each of the "printed" values in the args list.
sys.displayhook = self.append_arg
Copy link
Collaborator

@jcheng5 jcheng5 Nov 8, 2023

Choose a reason for hiding this comment

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

We should consider just replacing sys.displayhook once, at process start time, with a smart version of our own. And then have that smart version look at a different variable, backed by a contextvar, to decide where things actually go. This would let us keep "displayhooks" straight even when using async logic.

Q: Would this work in Quarto? In JupyterLab?

@jcheng5 jcheng5 marked this pull request as ready for review November 9, 2023 18:24
@wch wch merged commit 4e314af into main Nov 9, 2023
@wch wch deleted the flat-mode branch November 9, 2023 18:25
schloerke added a commit that referenced this pull request Nov 28, 2023
* main:
  Update shinylive to v0.1.1 (#825)
  Deploy dev docs on GitHub Pages (#824)
  Add docstrings
  Check for 'app' object in Express mode
  Use path to app as part of entrypoint for Express apps (#816)
  Install htmltools from github in CI (#811)
  Change htmltools dependency to use version number
  For layout.sidebar, default to open="always"
  Fix CSS class name
  Make page_sidebar main area white
  Update VS Code settings
  For quarto apps, check for `import *` (#810)
  Restore workaround for type stubs from typing_extensions
  Add try-except so parsing doesn't result in error
  Move is_express_app to separate file
  Add support for express mode apps (#767)
  Un-pin pyright version and fix type issues (#800)
  chore: Penguin app updates (#798)
  bug(`accordion(multiple=)`): Pass in accordion ID into accordion panel objects for `multiple` functionality (#799)
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.

2 participants