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

fix(window.ipfs): stop injecting window.Buffer #732

Closed
wants to merge 1 commit into from

Conversation

lidel
Copy link
Member

@lidel lidel commented Jul 10, 2019

This PR removes code responsible for injection of window.Buffer when window.ipfs is enabled.
We did that as a convenience tweak to make it easier to write demos/pocs, but polluting global scope introduced some issues on websites (Closes #637).

Question: should we

@lidel lidel requested review from alanshaw and hugomrdias July 10, 2019 23:04
@@ -46,7 +45,4 @@ function createWindowIpfs () {
return freeze(proxyClient)
}

// TODO: we should remove Buffer and add support for Uint8Array/ArrayBuffer natively
// See: https://github.com/ipfs/interface-ipfs-core/issues/404
window.Buffer = window.Buffer || _Buffer
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 support for TypedArray and ArrayBuffer before we ship this.

I'd be interested to know if this covers all the bases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've been thinking about it and (C) feels like a right way to do it.

Something I realized is that the first thing people do when they play with API is to add some text (String) to IPFS. What if we remove friction of converting to bytes and accept String input and convert it internally to a typed array?

afaik something like https://encoding.spec.whatwg.org would do:

const enc = new TextEncoder() // implicit utf-8
const bytes = enc.encode('This string will be converted to a Uint8Array'))

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I've wanted this for a while and allowed it in https://github.com/ipfs-shipyard/ipfsx/blob/master/API.md#example-1

Copy link
Member

Choose a reason for hiding this comment

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

The tricky bit with it is destinguishing between content and a path in the file system, which I think is supported somewhere

Copy link
Member

Choose a reason for hiding this comment

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

yeah thats the problem addFromFs

Copy link
Member Author

@lidel lidel Jul 11, 2019

Choose a reason for hiding this comment

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

Is addFromFs the only place?
I think it is quite intuitive special case where String is interpreted as file path. It could run the generic normalizeInput like every other command and then convert bytes back to file path internally.

Copy link
Member

Choose a reason for hiding this comment

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

Previously we had everything in the add, addReadable, addPullstream when we introduced addFromFs i think we kinda separated the /file/path input to this method only need further investigation

@lidel
Copy link
Member Author

lidel commented Apr 5, 2020

Superseded by #777

@lidel lidel closed this Apr 5, 2020
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.

Do not inject window.Buffer
3 participants