Consider using functions instead of getter properties

[mhart]

This is most definitely a style preference, but I think many newcomers will be confused by the use of dynamically defined properties that return promises here, and I think they could lead to bugs due to their magicness.

Is there a rationale for preferring:

let html = await r2('https://www.google.com').text

over

let html = await r2('https://www.google.com').text()

Especially considering that the latter aligns with the Fetch API that users might be familiar with?

Since the promises returned here perform I/O, I think it's a stronger signal to the reader to use functions instead of properties.

It's a relatively well accepted maxim in languages that have had property methods for a while (eg, C#) that they should still be inexpensive wrappers to internal state instead of performing any real work – as this goes against users' expectations of what a property is.

Again, totally just a personal preference – but I think it falls well within the principle of least astonishment.

[wcarron27]

+1

[MaikuMori]

The work is done by r2(), text property accesses data and doesn't do real work.

[mhart]

@MaikuMori

The work is done by r2(), text property accesses data and doesn't do real work.

I don't think that's true – it delegates to Body.text() which reads the network stream to completion: https://github.com/mikeal/r2/blob/fe1ad66ac955840809f5f47f6024546a5b459b25/index.js#L57

[mhart]

@MaikuMori but your comment absolutely indicates how many developers could think that 😉

[ljharb]

It's also true that getters and setters are much, much slower than using a simple function-valued property - even in TurboFan, I believe.

[MaikuMori]

@mhart Still fetch is the method getting the data. .text just copies response stream and gets text. Response has been already downloaded at that point.

Correct me if I'm wrong.

[mhart]

@MaikuMori yeah, that's wrong – fetch doesn't retrieve the body of the response, only the status code and headers. Methods such as text(), json(), etc are the ones responsible for actually streaming the response body, as described here: https://fetch.spec.whatwg.org/#concept-body-consume-body

You can test this by fetching a very large file – the initial fetch promise will return quickly (with whatever latency the initial connection requires), and you'll have your response object that you can get the status or headers from – but if you want the large body, you'll need to resolve one of the body methods, and that will stream the rest of the response over the network.

[MaikuMori]

@mhart Cool! Thanks for explaining! Now I totally agree with you.

[ljharb]

Another concern with getters/setters is that if you make a typo, you silently get undefined versus getting a TypeError. This problem is rampant in BDD-based testing systems that use noop matchers, like expect(x).to.exist - since expect(x).to.be.yogurt will merrily pass, despite not being a matcher - and it causes a ton of bugs in the form of tests silently failing to test what you think they’re doing.

Hopefully someone typing .josn or .test would be caught in testing, but t drastically minimizes the chance of an easy mistake to make if these are all functions.

[gr2m]

we will change the API to functions, just doing some spring cleaning first 🛁✨