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

Remove Object.setPrototypeOf() #3306

Merged
merged 5 commits into from
May 14, 2018
Merged

Remove Object.setPrototypeOf() #3306

merged 5 commits into from
May 14, 2018

Conversation

seklyza
Copy link
Contributor

@seklyza seklyza commented Apr 12, 2018

Fixes #3236

Object.setPrototypeOf() does not work on JSC (Android). Instead, we're setting the prototype of this manually.

seklyza added 2 commits April 12, 2018 16:57
Object.setPrototypeOf() doesn't work on Android
@apollo-cla
Copy link

apollo-cla commented Apr 12, 2018

Warnings
⚠️

There are library changes, but not tests. That's OK as long as you're refactoring existing code

Messages
📖

Please add your name and email to the AUTHORS file (optional)

Generated by 🚫 dangerJS

@ianks
Copy link

ianks commented Apr 21, 2018

This LGTM for compatibility purposes. In a future PR, when should reconsider setting the prototype directly as it will cause JITs to deoptimize.

@Diego-Franco
Copy link

@seklyza can you resolve your conflicts and then merge the PR, please??

@ujwal-setlur
Copy link

ujwal-setlur commented May 13, 2018

Is this going to get cleaned up and merged? Only conflict is CHANGELOG.md

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this PR @seklyza! I think we're almost ready to go here. I'll take care of the review items, and we'll plan on getting this merged shortly. Thanks!

# Change log

### vNext

- Fixed an issue involving `Object.setPrototypeOf()` not working on JSC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've adjusted the comment here to be a bit more descriptive.

@@ -65,6 +65,6 @@ export class ApolloError extends Error {

this.extraInfo = extraInfo;

Object.setPrototypeOf(this, ApolloError.prototype);
(this as any).__proto__ = ApolloError.prototype;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add a comment here explaining why we aren't using Object.setPrototypeOf. Otherwise someone might add it back in the future.

@hwillson hwillson merged commit cb44481 into apollographql:master May 14, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants