Skip to content
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.

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

std/yaml no longer exports Schema in a usable way? #6037

Closed
cscheid opened this issue Sep 23, 2024 · 9 comments
Closed

std/yaml no longer exports Schema in a usable way? #6037

cscheid opened this issue Sep 23, 2024 · 9 comments
Labels
bug Something isn't working yaml

Comments

@cscheid
Copy link
Contributor

cscheid commented Sep 23, 2024

In deno.land's std/yaml, we can write:

$ deno
Deno 1.46.3
exit using ctrl+d, ctrl+c, or close()
REPL is running with all permissions allowed.
To specify permissions, run `deno repl` with allow flags.
> import { Schema } from "https://deno.land/std@0.224.0/yaml/schema.ts";

In jsr, it seems like the Schema import has been hidden. Note

https://jsr.io/@std/yaml/1.0.5/parse.ts#L9

This is a problem for libraries that need to support new YAML schemas, since that constructor is necessary in these cases. (specifically, resolution of ! rules with new explicit types). In Quarto, we used to be able to write code like this:

https://github.com/quarto-dev/quarto-cli/blob/main/src/core/yaml.ts#L264-L279

And now it seems that without these exports, we'll have to ship our own port of js-yaml in the way that JSR chose to not export these symbols? Is that something that can be reconsidered? Thanks in advance.

@iuioiua
Copy link
Contributor

iuioiua commented Sep 23, 2024

Hi @cscheid, we replaced use of the Schema class option with the SchemaType type in v1 of @std/yaml. First off, would you be able to provide an example of the YAML output you'd like to parse and the corresponding expected object?

@iuioiua iuioiua added yaml and removed needs triage labels Sep 23, 2024
@cscheid
Copy link
Contributor Author

cscheid commented Sep 24, 2024

It comes up all the time in the configuration objects we have to parse. A minimal example is something like

eval: !expr "3 + 4 + runif(10)"

SchemaType provides a fixed enumeration. Schema works as a constructor that we have control over; they're not equivalent. Did you take a look at the link I shared with the current usage from the yaml library deno.land/std?

https://github.com/quarto-dev/quarto-cli/blob/92cad4456c4e8b635979d6b39f45b507aba70edb/src/core/yaml.ts#L264-L279

Specifically, we need a new entry in the explicit list for the Schema constructor that uses a custom Type to return a particular JSON object from the tagged expression. That is supported by js-yaml but is explicitly not supported in deno's version because Schema is not exported. That seems unnecessary.

@iuioiua
Copy link
Contributor

iuioiua commented Sep 24, 2024

SchemaType provides a fixed enumeration. Schema works as a constructor that we have control over; they're not equivalent.

That's correct, they're not equivalent. For context, we made the move to simplify the API of yaml while still retaining most of the functionality. At the time, we were aware of the trade-off of losing the ability for the user to define custom types, but weren't able to find any instances of that happening.

Did you take a look at the link I shared with the current usage from the yaml library deno.land/std?

Yes - I just wanted to be clear on the YAML input and expected result 🙂. We also found:

We're considering adding a types: Type[] to parse() that extends type-handling functionality on top of the currently defined schema: SchemaType option. WDYT?

@cscheid
Copy link
Contributor Author

cscheid commented Sep 24, 2024

I think I don't understand why exposing Schema is not an option. It's simpler, keeps the code closer to js-yaml, and restores functionality that used to exist until relatively recently.

Overall, in reflecting my experience in chasing Deno's breaking stdlib changes while trying to ship Quarto to users who don't want their TS scripts to change, I confess that I wish that deno's stdlib were opting for a more conservative approach wrt backwards compatibility.

@kt3k
Copy link
Member

kt3k commented Sep 24, 2024

@cscheid

Schema class doesn't exist anymore in the code base (It is now an interface). We considered js-yaml's abstractions of Schema and Type are not well designed and not well used. So we removed them from the public interface at @std/yaml v1 release.

Currently we are considering to add types option to restore the customized type feature (#6042). Your usage will be translated in something like:

parse(input, { schema: "json", types: [{
  tag: "!expr", {
  kind: "scalar",
  construct(data): Record<string, unknown> {
    const result: string = data !== null ? data : "";
    return {
      value: result,
      tag: "!expr",
    };
  },
}]);

Do you think this could be a replacement of your usage?

We'd prefer to keep Schema type private because we'd like to keep the API surface as minimal as possible. In your use case, do you need to expose QuartoJSONSchema to your users?

@cscheid
Copy link
Contributor Author

cscheid commented Sep 24, 2024

Do you think this could be a replacement of your usage?

In an immediate sense, it would, once it's available.

We considered js-yaml's abstractions of Schema and Type are not well designed and not well used

It's your prerogative to do so, but as a result, you've broken user code without a fix in the meantime. I've had to move away from Deno's stdlib into js-yaml itself for a feature that used to exist. That's not great.

I'd like to reiterate my concern about deno's stdlib API churn. Quarto tries really hard to maintain backwards compatibility out of respect for our users time and desire for their scripts to remain usable over Quarto version upgrades. That's becoming a significant source of toil in our code base. We have started to protect ourselves against it by writing a RAL that keeps our internal API consistent against Deno stdlib's changes, and I wish that weren't necessary.

@timreichen
Copy link
Contributor

It's your prerogative to do so, but as a result, you've broken user code without a fix in the meantime.

Is using a pre 1.x version an option until this is implemented?

@cscheid
Copy link
Contributor Author

cscheid commented Sep 24, 2024

Is using a pre 1.x version an option until this is implemented?

Maybe, but it's not an ideal situation for us, because selectively holding back parts of deno's stdlib cause other issues that we've experienced in the past. Specifically, there are enough cross-imports in Deno's stdlib that it is hard to import one part of the stdlib under one version and another under a different version. In addition, Deno's stdlib often follows specific Deno versions, and we've had trouble with holding back previous versions because they no longer typecheck on newer Deno versions.

I imagine this is an issue that might be only come up when large projects use a lot of the stdlib. Quarto imports a lot of it, and so we find ourselves needing to do a lot of massaging to make it all work.

In any case, we removed the dependency on std/yaml from Quarto's codebase.

@cscheid cscheid closed this as not planned Won't fix, can't repro, duplicate, stale Sep 24, 2024
@kt3k
Copy link
Member

kt3k commented Sep 25, 2024

We recently stabilized (published v1 of) many of packages. (See #4600 )

We'll be very conservative with those stabilized packages, and we try to keep backward compatibilities of those package as far as possible. However you might sometimes see breaking changes with unstable packages (ones with version 0.x.y) or experimental APIs (they are exported from unstable- prefixed paths).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working yaml
Projects
None yet
Development

No branches or pull requests

4 participants