Comments (11)
perhaps we can have a test case for this to find a more optimal path, perhaps one that avoids Object.keys()
if that's the root problem
from node.
This is a somewhat contentious issue.
IMO, I agree with the current behavior -- Node/IO apis should only take the given object into account, and ignore its [[Proto]] chain. That said, the TC should probably make an authoritative decision about "how Node/IO deals with API parameters" -- ideally something that can be put into the docs.
from node.
I'd rather support proto chain, because sometimes it's very convenient to have. Imho, this proto getters case is a very strong argument to do so.
According to my benchmarks, Object.create()
is 2 times faster than util._extend()
on options object with no properties and the more properties there are, the worse util._extend()
gets.
url.parse()
output object contains 12 properties, and Object.create()
outperforms util._extend
on it 6 times (4.2M ops/sec
vs 0.6M ops/sec
).
So it's faster even if we're talking plain js objects. But urlparser
tries to improve speed with lazy getters, and current util._extend()
implementation practically forbids you to do this kind of performance optimizations.
This is the reason I'm usually using Object.create()
whenever I'm trying to modify options object leaving original function arguments intact.
I suppose this is an old code, and v8 didn't have Object.create()
at the time when it was written. That's why we never give much thought to this. But I think it's about time to discuss what the best practice here is.
from node.
I suppose this is an old code, and v8 didn't have Object.create() at the time when it was written. That's why we never give much thought to this. But I think it's about time to discuss what the best practice here is.
I do not think it would be safe to assume that the absence of its usage in Node's APIs for options objects is due to the lack of Object.create
. Object.create
(and polyfills for Object.create
) have been around for a long time.
url.parse() output object contains 12 properties, and Object.create() outperforms util._extend on it 6 times (4.2M ops/sec vs 0.6M ops/sec).
While performance has always been a guiding concern of Node, there has also long been a desire to make the API more explicit where possible, even at the cost of CPU cycles. I can envision a situation where {host: 'some-host'}
is linked to an object specifying hostname
, and requests end up going to an unexpected location. Trying to debug that would be more than a little maddening, since even console.log
'ing the object is not going to display the parent object's hostname
property.
from node.
I can envision a situation where {host: 'some-host'} is linked to an object specifying hostname
I did shoot myself in a foot once, when I had an option object like opts={uri: 'foo'}
and was setting opts.url='bar'
after that (request
library supports both). No prototypes involved.
The fact that you can specify the same thing in two different ways is an api issue, which has nothing to do with prototype chains imho.
Trying to debug that would be more than a little maddening, since even console.log'ing the object is not going to display
I suppose an addition to console.log
which says "hey, this object has non-standard prototype (for example, instead of { host: 'blah' }
displaying {* host: 'blah' }
or < host: 'blah' >
or { host: 'blah', <proto-chain-length>: 2 }
would help this a bit.
from node.
If you outlaw the proto chain on options objects, it will cause breakage on legacy code.
I wonder if in the specified case, Object.create(Object.freeze(options || {})) might cause V8 to optimize the case (eg it can rely on static rather than dynamic lookups on the proto chain) - and if not it may be a case that v8 optimize in the future
from node.
I feel somewhat strongly that you should always follow the prototype chain, i.e. just use [[Get]]. This is the most natural pattern, i.e. var x = options.x
.
Another way of thinking about this is that if you were to write such options-taking functions in ES6 you would likely do
function client({ agent, protocol, ... }) {
/* use agent, protocol, etc. */
}
Probably http.client itself couldn't work this way because it has so many overloads and defaults, but it should still be useful semantic guidance.
from node.
From TC meeting, #144:
@bnoordhuis: continue in the issue tracker
@indutny and @piscisaureus also expressed opinions on this but there was no resolution, just an agreement that there shal be further bikeshedding. I'm going to remove the tc-agenda label for now but if this keeps on going on and doesn't get anywhere productive then the label could be put back for a future meeting.
from node.
feels good to know you guys discussed this at least. 🙌 io.js 🙌
from node.
/cc @moll
from node.
Closing this. There's no definitive resolution from the TC, but @domenic swayed me to the proto-side via the ES6 destructuring argument. If anyone notices any specific APIs that are not accepting inherited attributes, feel free to open a PR and cc me. I'd be happy to take a look at them, and am generally +1 on making those sorts of changes.
from node.
Related Issues (20)
- Importing module on filesystem from data URL throws `TypeError: Invalid URL`
- Can you please change the commit message to something like: `crypto: use correct object on assert`
- RangeError when a file reaches around 120K lines of a simple repeating const variable HOT 6
- ERR_ACCESS_DENIED with Experimental Permissions and Relative Paths HOT 2
- Add support to Buffer.indexOf(value, startOffset, endOffset) HOT 3
- compiling node on openBSD
- High cpu usage and traffic when using windows named pipes
- Node 20.11.1 doesn't contain libuv 1.48 HOT 2
- test runner - Basic array join test doesn't finish before parent. HOT 4
- Support MockAgent for non-undici requests as well HOT 3
- Print a "current stack/callback/promise dump" from a running program HOT 2
- `os.homedir()` on Windows returns non-existent path HOT 1
- shellJS.exec() or child_process.exec() not executing callback function HOT 5
- stream.compose(...) doesn't destroy all active composed streams when it is destroyed
- fs.createWriteStream can cause out-of-order writes, in v18.16+ HOT 3
- test_runner: t.after is never called HOT 9
- Snapshot run failed on special RegExp HOT 5
- Throws assertion with match delegation
- test_runner (after) doesn't work on v21.7.0 HOT 1
- breaking change in 21.7.0 when mocking fetch HOT 4
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from node.