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

Could symbol be added to the prototype instead of instace ? #25

Open
Gozala opened this issue Jun 10, 2020 · 8 comments
Open

Could symbol be added to the prototype instead of instace ? #25

Gozala opened this issue Jun 10, 2020 · 8 comments

Comments

@Gozala
Copy link

Gozala commented Jun 10, 2020

No description provided.

@satazor
Copy link
Contributor

satazor commented Jun 11, 2020

I think it can! However, while it will increase instances creation performance, it will decrease isX invocation performance, as it will have to fallback to the prototype.

Not sure what to do here.

@Gozala
Copy link
Author

Gozala commented Jun 11, 2020

Sorry I submitted this in a rush, and forgot to provide more context. The reason this came up is I'm having to do this nasty bit of a hack:

https://github.com/ipfs/js-ipfs/pull/3081/files#diff-833dfdb70fcd89365dc3bddf93f78e7eR57

Which is needed because once CIDs cross thread boundaries their prototype chain is lost. Code under the link restores those prototype chains. However in case of CID turns out it is not enough because CID.isCID(v) is primarily how checks are performed, and it seems to return false even when v instanceof CID would return true, because that symbol field is set on instance rather than prototype. Which in turn is lost because symbols don't carry over during structure cloning.

When I started looking into this, I got wondering why that field is set on instance vs prototype and created this issue.

I'm not sure if moving symbol field is a best or adequate way to get rid of that nasty code I've linked to, but that was my immediate thought.

@hugomrdias
Copy link

@Gozala probably better to remove class-is and just use validateCID.

@Gozala
Copy link
Author

Gozala commented Jun 11, 2020

@Gozala probably better to remove class-is and just use validateCID.

@hugomrdias remove from where ? All of the code base ?

@hugomrdias
Copy link

I was suggesting cid

@Gozala
Copy link
Author

Gozala commented Jun 11, 2020

@hugomrdias sorry I'm not sure I understand what you mean by:

@Gozala probably better to remove class-is and just use validateCID.

Remove class-is from cids library ? If so do you mean also remove isCID static method or implementing it differently ? If former there's a lot of code that uses isCID and probably going to be an impractical to do that. If later would be good to know what you have in mind.

@Gozala
Copy link
Author

Gozala commented Jun 11, 2020

I think it can! However, while it will increase instances creation performance, it will decrease isX invocation performance, as it will have to fallback to the prototype.

Not sure what to do here.

Another alternative could be to set it on both prototype and instance.

@hugomrdias
Copy link

Keep the api just without class-is, implement isCID with instanceof with fallback to validating the components

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

No branches or pull requests

3 participants