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

Proxy Interface for Document #294

Open
pskfyi opened this issue Jul 10, 2021 · 7 comments
Open

Proxy Interface for Document #294

pskfyi opened this issue Jul 10, 2021 · 7 comments
Labels
enhancement New feature or request

Comments

@pskfyi
Copy link

pskfyi commented Jul 10, 2021

The round trip for editing documents while preserving comments is understandably complex. It seems plausible to wrap YAML.Document.Parsed in a Proxy which can be read similarly to the results of .toJs() (or .toJson()), but setting properties on it uses methods like .setIn() under the hood to keep the full document structure up-to-date.

# Hello World
foo:
  bar: baz
const doc = YAML.parseDocument(/* from above */).toProxy()
doc.foo.bar // "baz"
doc.toJson() // { "foo": { "bar": "baz" } }
doc.foo.bar = "qux"
doc.toString()
/*
 * # Hello World
 * foo:
 *   bar: qux
 */

I intend to prototype this in the next few weeks and I wanted to see if the library would be interested in a feature like this, or perhaps if you've explored this and found some problem with the approach. If it seems plausible, I'd like to collaborate.

Cheers 🍻

@pskfyi pskfyi added the enhancement New feature or request label Jul 10, 2021
@eemeli
Copy link
Owner

eemeli commented Jul 10, 2021

That sounds like a really interesting idea, I'm definitely interested in seeing what your prototype might look like.

While I had not considered a Proxy before, I had thought of optionally including some hidden fields (symbol-keyed, non-enumerable) in the toJS() output that could retain metadata if re-yamlified, but hadn't come up with a really clean way of doing so. Proxies sound like a better solution.

@pskfyi
Copy link
Author

pskfyi commented Jul 10, 2021

I will report back with a repo link when I have something to show.

I had thought of optionally including some hidden fields (symbol-keyed, non-enumerable) in the toJS() output that could retain metadata if re-yamlified

I'm betting you've seen this but if not it's worth a look: https://www.npmjs.com/package/comment-json

@coreybutler
Copy link

I feel it is important to point out that there are still JS runtimes that do not support Proxy. To be more specific, Cloudflare workers don't support it. I currently use this library to convert JSON to YAML (and vice versa) in an API service running in a worker. I wish this weren't the case, because the Proxy pattern is a solid one.... but if this goes forward, consider the ramifications (perhaps a semver major change).

@pskfyi
Copy link
Author

pskfyi commented Jul 14, 2021

@coreybutler good to raise this issue. Are you certain it's not supported in Cloudflare workers? Per their documentation on JavaScript:

All of the standard built-in objects supported by the current Google Chrome stable release are supported, with a few notable exceptions:

None of the exceptions refer to Proxy or Reflect, both of which are listed in the referenced list of standard built-in objects.

Proxies are from ES6/ES2015, and yaml@2 seems to be presently targeting ES2017.

@coreybutler
Copy link

@pskfyi - I'm actually very happy to be outdated on this matter. Their docs have always stated support for Proxy & Reflect, but it never actually worked... until I ran the same test again this morning. I just started using them about 8 weeks ago and couldn't get a basic Proxy object to work, so something has changed. Their workers are based on v8 Isolates and only mirror Chrome's standard built-in objects (i.e. they have to recreate alot). It seems as though the implementation details have now been worked through though... at least enough where the basic proxy traps work (i.e. get/set) in a simple test.

Anyhow, I'd be happy to test a Proxy-based implementation of this in Cloudflare Workers when it is available.

@eemeli
Copy link
Owner

eemeli commented Jul 14, 2021

As a clarification of my position, if a Proxy interface is added, it should probably first be added in parallel with the existing API, rather than replacing it.

@pskfyi
Copy link
Author

pskfyi commented Jul 14, 2021

Understood, and I agree completely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants