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

Rhino/RingoJS compatibility fix for lacking __proto__ feature / Object.setPrototypeOf polyfill #51

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

epistemery
Copy link

No description provided.

@kriszyp
Copy link
Member

kriszyp commented Nov 16, 2013

I had a few questions about this:
Is this currently broken on the latest versions of Rhino/Ringo (can we no longer enable the proto feature at the VM level)?
Is there a reason for calling it ringojs-compat if it is polyfill for node as well?
Could you do an early test of proto and binding of the setPrototypeOf function so that proto doesn't have to be tested for each setPrototypeOf call?
Do you have (Dojo) CLA filed? (probably not necessarily for this patch, but would be nice to have)

@epistemery
Copy link
Author

I did not check whether there was a way to enable the proto feature since I assumed it is considered deprecated. I saw the comment in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf#Notes and thought it would be good to use Object.setProtoypeOf. Maybe there is a way to re-enable the proto feature in Rhino (http://www-archive.mozilla.org/rhino/apidocs/org/mozilla/javascript/Context.html#FEATURE_PARENT_PROTO_PROPERTIES) but I have no idea how easy it is to run ringojs with this flag enabled.

This post was helpful: http://stackoverflow.com/questions/10476560/proto-when-will-it-be-gone-alternatives To be honest I skipped some of the comments, so maybe there is a better way to do the same thing.

There is no reason to call it ringojs-compat, it just seemed right at the time and I didn't know where else to put it.

Of course I should do an early test of proto, thanks! I will commit the changes some time this week.

I currently have no CLA, maybe I should do that, haven't thought of that. Sorry if I caused any troubles and please feel free to ignore the whole thing if you'd like to solve it differently...

Maybe you could tell me where else in the persevere stack the proto feature is used. Would help me a lot.

@kriszyp
Copy link
Member

kriszyp commented Nov 25, 2013

I agree that patching proto with a more standard function is a worthwhile upgrade.

The proto property is used in Pintura in jsgi/metadata.js and facet/jsgi.js. It would be pretty trivial to require the patch module in those modules, I could certainly add that.

Also, BTW, here is the Dojo CLA (we want this for any significant patches to keep the codebase clean): http://dojofoundation.org/about/cla

@chaorace
Copy link

Thanks for writing this Shim btw!
I wanted to comment here so others in my situation could find this more easily. Service Now runs on an ancient Rhino version and this shim was exactly what I needed to get Babel transpiled code working. (see also: the coreJS babel config for getting Babel output that doesn't constantly crash on Rhino)

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

Successfully merging this pull request may close these issues.

3 participants