pyscript: [next] doesn't handle syntax errors well

I have noticed that when <py-config> contains invalid TOML, we have several unexpected things, but I could not find a common pattern so I’m reporting all of them here.

Case 1: invalid syntax

<!doctype html>
<html lang="en">
    <head>
        <meta charset="UTF-8" />
        <meta name="viewport" content="width=device-width, initial-scale=1.0" />
        <title>PyScript Error</title>
        <script type="module" src="https://cdn.jsdelivr.net/npm/@pyscript/core@latest"></script>
        <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@pyscript/core/dist/core.css">
        <py-config>
          [[fetch]]
              files = ["pyterminal.py"]this is a syntax error
        </py-config>

        <script type="py">
          print('hello')
        </script>
    </head>
</html>

Here I correctly get an error in the console:

However:

errors should be displayed to the DOM by default
the error message says something about JSON which is very confusing, since <py-config> is supposed to contain TOML

Case 2: another invalid syntax, completely ignored

        <py-config>
          this is invalid syntax
        </py-config>

        <script type="py">
          print('hello')
        </script>

In this case, the py-config is just silently ignored and hello is normally printed. I would expect that in case of syntax error in the config, the execution of the page stops and the error is displayed to the DOM.

Case 3: I cannot really understand what’s happening

        <py-config>
          [[fetch]]
          files = 
        </py-config>

        <script type="py">
          print('hello')
        </script>

Here I get yet another error, but also I see that it tries to fetch config.txt, which I don’t really know where it comes from 🤔

About this issue

Original URL
State: closed
Created 10 months ago
Comments: 37 (37 by maintainers)

Most upvoted comments

So I went ahead and re-published that module in a CDN compliant way: https://github.com/jakwings/toml-j0.4/issues/9#issuecomment-1720963049 … I will change the previous one to use this one instead.

WebReflection on Sep 15, 2023

I just changed package.json to add the type definitions to the file list. The change has been upstreamed and released in toml-j v1.1.2

hoodmane on Sep 14, 2023

“sorry dude, the config is a bit bananas”

Can we have "Config is 🍌 (check your TOML)" as the error message? </joke>

ntoll on Sep 14, 2023

@antocuni to be clear - I wasn’t meaning to be disrespectful.

However, I think we’re in a very different place to when we last had the TOML/JSON discussion WRT config. I certainly support your point about discussing things… but bear in mind that the solution we have already supports TOML if only to a limited extent, and there is an undeniable performance advantage to using JSON (it’s already built in!) over TOML. In fact, if memory serves me correctly, the “next” effort for PyScript arose because @WebReflection was asked to look at the “classic” codebase with a view to making it smaller, more efficient and idiomatically web-ish so we started up quickly. A big factor in the slowness was that we had a full TOML parser to include.

To clarify my position - of course we should support TOML, that’s what we decided… and I agree with you that it would be disrespectful not to (hence my “as a courtesy to Pythonistas” point). But we also support JSON and (here’s the change in my position), the emphasis / recommendation should be to use JSON while acknowledging one could use TOML (that we parse on a best effort basis).

Ultimately, we need a focussed and friendly discussion to ensure we’re aligned with what we’re actually offering. I’m happy to facilitate this (I feel a Zoom call, or a section of PyScript FUN should be dedicated to this). The most important thing for me is that I trust you all - despite the various technical differences I know we have, I know we’ll all engage with the very best of intentions as we try to explore this puzzle. 👍

ntoll on Sep 14, 2023

I haven’t considered people writing bad configs as I consider myself that to be a very rare case

I think it’s much more frequent that you think 😃. I started this discussion because I wrote an invalid config and took 10 minutes to understand what was going on 😂

antocuni on Sep 14, 2023

Warning: very opinionated opinion coming… don’t judge (but I welcome constructive feedback and discussion - which I know I’ll get from you folks anyway) 😉

We’re in the world of the web. Let’s just use/promote JSON for config, and explain that as a courtesy to the Python world we make a “best effort” attempt to parse correct TOML - but there be dragons. 🐉

Size does matter in the world of the web (especially on mobile). Including a whole TOML parser is a fly in the ointment for end users who expect snappy startup time (i.e. if we include a TOML parser, we’re blocked from doing anything until said parser has been downloaded in order to parse the config).

“We recommend using JSON” = JSON is already built in, and since we’re in the context of a browser, JSON shouldn’t feel an odd choice. Furthermore, I like the “but as a courtesy to Python we make a best attempt at TOML” because it promotes choice without too much cost.

</end opinionated opinions>

😛

ntoll on Sep 14, 2023

Please bear in mind that the TOML is parsed to JSON (hence the JSON related errors). I believe we should be rather unforgiving when parsing TOML->JSON and just fail at all costs with something like “Invalid TOML in configuration”.

ntoll on Sep 13, 2023