search

TritonDataCenter / node-manta

Node.js SDK for Manta
75 stars 54 forks source link

MANTA-4341 Optimize conditional object operations #382

Closed chudley closed 4 years ago

chudley commented 4 years ago

These are the tests I've been using the check my work on MANTA-4341. It's not totally clean yet, and I'm still not sure about things like the lack of res.statusCode on a PreconditionFailedError.

I wanted to wrap it up into a PR though because the other parts of MANTA-4341 are due to be reviewed and it'll be good to at least prove there's been some testing. Perhaps it'll also be good to get some early review as well to make sure it's not a million miles off.

chudley commented 4 years ago

Ah, looks like that property is only available from Node v13 (https://github.com/nodejs/node/pull/28814).

I'll see if there's a better way to verify this behaviour in the test suite.

chudley commented 4 years ago

So we can't get at these properties in earlier versions of node unless we reach into _readableState ourselves. It turns out lots of people do it anyway.

I haven't put a great deal of thought into these tests so there's a good chance they're not testing the right thing anyway. I've removed them in this latest push, but I'd be happy to revisit if you think otherwise!

kellymclaughlin commented 4 years ago

I haven't put a great deal of thought into these tests so there's a good chance they're not testing the right thing anyway. I've removed them in this latest push, but I'd be happy to revisit if you think otherwise!

I think it's probably ok to leave them out versus reaching into _readableState. I feel like the other assertions should give us confidence that the operations are working as intended so hopefully it's ok without these checks.