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

Add support for Uint8Array's in place of node Buffer's to ipfs-unixfs-importer #56

Closed
Gozala opened this issue Jul 7, 2020 · 6 comments
Assignees
Labels
status/ready Ready to be worked

Comments

@Gozala
Copy link
Contributor

Gozala commented Jul 7, 2020

I glanced through the implementation and it seems that dependence is inherited from use of following libraries

There are few lines that also use Buffer directly but there Uint8Array would have done just fine as well.

Trying to actually remove here might be difficult, however we could probably just remove it from the API surface and turn Uint8Arrays into Buffers internally where needed.

@Gozala Gozala added the need/triage Needs initial labeling and prioritization label Jul 7, 2020
@Gozala
Copy link
Contributor Author

Gozala commented Jul 7, 2020

Actually I think I have overlooked chunk validation phase that seems to normalize each chunk

for await (const content of source) {
if (content.length === undefined) {
throw errCode(new Error('Content was invalid'), 'ERR_INVALID_CONTENT')
}
if (typeof content === 'string' || content instanceof String) {
yield Buffer.from(content, 'utf8')
} else if (Array.isArray(content)) {
yield Buffer.from(content)
} else {
yield content
}

Although I'm also starting to suspect that ipfs.add([ new ArrayBuffer(1024) ]) would cause content was invalid error because ArrayBuffer because ArrayBuffers seem to be passed through although they do not have .length property.

P.S.: I'll verify that assumption and submit test / fix as necessary.

@Gozala
Copy link
Contributor Author

Gozala commented Jul 7, 2020

@achingbrain is this the right type signature for the importer:

declare function importer(source:AsyncIterable<ImportEntry>, ipld:IPLDResolver, options?:ImporterOptions)

type ImportEntry =
  | ImportFile
  | ImportDir

type ImportFile = {
  path: string|void,
  content: Content,
  mtime?: Mtime,
  mode?: Mode
}

type ImortDir = {
  path: string,
  content?:void,
  mtime?: Mtime,
  mode?: Mode
}


type Content =
  | Iterable<string>
  | Iterable<ArrayBuffer>
  | Iterable<ArrayBufferView>
  | AsyncIterable<string>
  | AsyncIterable<ArrayBuffer>
  | AsyncIterable<ArrayBufferView>

type Mode = string | number
type MTime = number | Date | UnixFSTime | HRTime
type UnixFSTime = {
  secs: number,
  nsecs?: number 
}
type HRTime = [number, number]

type IPLDResolver = {
 // ...
}

type ImporterOptions = {
  // ...
}

@achingbrain achingbrain added status/ready Ready to be worked and removed need/triage Needs initial labeling and prioritization labels Jul 16, 2020
@achingbrain
Copy link
Member

The type it expects to be yielded by the source is:

{
  path: String,
  mode: Number (optional),
  mtime: {
    secs: Number,
    nsecs: Number (optional)
  },  (optional)
  content: AsyncIterator<Buffer> (optional)
}

It handles more, as you found, maybe it shouldn't.

@Gozala
Copy link
Contributor Author

Gozala commented Jul 16, 2020

In that case can we loosen up it a bit to accept Uint8Arrays in place of buffers ?

@achingbrain
Copy link
Member

Of course, we shouldn't require the input to be Buffers anywhere. I think the only place that might get painful is the use of bl, which we have to avoid unnecessary copying, but as long as we end up with similar performance characteristics then all good.

@Gozala Gozala self-assigned this Jul 16, 2020
@Gozala
Copy link
Contributor Author

Gozala commented Aug 6, 2020

fixed by #69

@Gozala Gozala closed this as completed Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/ready Ready to be worked
Projects
None yet
Development

No branches or pull requests

2 participants