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

Use setPrototypeOf #4

Open
wesleytodd opened this issue Jan 31, 2016 · 14 comments
Open

Use setPrototypeOf #4

wesleytodd opened this issue Jan 31, 2016 · 14 comments

Comments

@wesleytodd
Copy link
Member

Hey, just perusing the repo's around here and noticed in this one it is setting __proto__. Not sure what the end use of this module will be, but we had a discussion around this here because that breaks some browser support. Also, there is a new and standardized api to do this, Object.setPrototypeOf. So the router is using this module to get a nice fallback.

If you want I can PR that in, but I wanted to open this to make sure I wasn't misunderstanding the use case for this module :)

@Fishrock123
Copy link
Member

When @dougwilson has previously tested this, __proto__ was the most efficient in V8 / node.

@wesleytodd what browsers don't support __proto__? As far as I understand all current ones do and will continue to do so due to usage.

@wesleytodd
Copy link
Member Author

IE10 was the main one, but I guess since MS just dropped support for anything below 11, maybe this is a moot point. Technically according to MDN __proto__ has been standardized as legacy:

Warning: While Object.prototype.proto is supported today in most browsers, its existence and exact behavior has only been standardized in the ECMAScript 6 specification as a legacy feature to ensure compatibility for web browsers. For better support, it is recommended that only Object.getPrototypeOf() be used instead.

The original issue I linked to was about a year ago, and I was trying to support back to IE9 with the router. Anyway, totally understand if you want to close this.

EDIT: Or maybe if we are sure the perf is better I could switch that module to prefer the __proto__ version?

@Fishrock123
Copy link
Member

@wesleytodd what exactly are you trying to run in a browser that requires this functionality?

@wesleytodd
Copy link
Member Author

Hopefully most what what express is doing. I am currently running just the router, but in the future I would love to be able to run something that looks and functions just like an express app on the backend. That is the only reason I even mentioned this issue here is because I assume this module will take over this (https://github.com/strongloop/express/blob/5.0/lib/express.js#L43-L44) in the future. Am I correct in assuming that?

@Fishrock123
Copy link
Member

Correct. If you make a PR I'll review it but I don't really make any guarantees about browser support from myself.

@wesleytodd
Copy link
Member Author

Thats what I'm here for, browser support :)

And FWIW, just ran this jsperf and the perf is comparable in up to date chrome: http://jsperf.com/new-vs-mutable-proto/2

@Fishrock123
Copy link
Member

@wesleytodd Two things:

  1. There will be up to like, 100 properties attached, not just from express, in a busy application.
  2. They can't really be defined in an object literal I think, most will be defined after the fact, causing some amount of deopt.

@wesleytodd
Copy link
Member Author

  1. updated that jsperf to add 200 properties, still same perf characteristics. http://jsperf.com/new-vs-mutable-proto/5
  2. This might be an issue, since the fallback iterates over the properties. Do you have a link to somewhere that does this so I can see?

@Fishrock123
Copy link
Member

@wesleytodd Currently in express: no, but that is what this library does because it makes this a usable API.

Some background: setPrototypeOf() seems fairly new, I only heard about it around a week ago. Either way, it's definitely not in node <4. The alternatives had been either __proto__ or some sort of Object.setProperty(). The latter was much slower.

From what I can tell __proto__ and setPrototypeOf() are possibly aliases in some form.

@wesleytodd
Copy link
Member Author

Yeah, I guess its newish, here is the best source for info on it that I know of: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf

It was spec'd out in es2015, but that was the point of that module I linked to originally. To get the new feature while keeping backward compat.

For reference, I am currently working on building a express compatible implementation for the browser. It would hopefully encompass as much of the express api surface area as makes sense run in the browser. So supporting the ability to customize req/res in the same way express allows would be awesome. That is why I care about this.

@Fishrock123
Copy link
Member

@wesleytodd are you interested in making a PR for it? :)

@wesleytodd
Copy link
Member Author

Absolutely, sorry, I got distracted and on other things. I can do that tonight or tomorrow when I get home.

@wesleytodd
Copy link
Member Author

See #6 :)

@dougwilson
Copy link

As far as my opinion on this, I'm more aligned with @wesleytodd, as that is the more up-to-date consensus since the last time you were around, @Fishrock123 :) The router discussion has a pretty relevant recent discussion on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants