-
Notifications
You must be signed in to change notification settings - Fork 8
feat: add support for optional unixfs file link writer in create file #62
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
base: main
Are you sure you want to change the base?
feat: add support for optional unixfs file link writer in create file #62
Conversation
8d68b24
to
2e2c1b7
Compare
2e2c1b7
to
a33ec24
Compare
a6023f4
to
cb99618
Compare
@@ -286,6 +307,7 @@ const encodeLeaf = function* ({ hasher, linker }, { id, content }, encoder) { | |||
const link = /** @type {UnixFS.FileLink} */ ({ | |||
cid, | |||
contentByteLength: content ? content.byteLength : 0, | |||
contentByteOffset: content ? content.byteOffset : 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
propagates offset to be able to map original content byte offset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided my feedback, reflecting how I would prefer going about this feature. Specifically I think there is a simpler path that does not introduce extra complexity & if I was maintaining this code I would requested to make proposed changes so it's easier to maintain the project. However, I'm not maintaining this code and therefor don't feel like it's my call to make, so my comments should be treated as recommendations and nothing more.
if (!state.unixFsFileLinkWriter) { | ||
return { | ||
state: newState, | ||
effect: Task.listen({ | ||
link: Task.effects(tasks), | ||
block: writeBlock(state.writer, block), | ||
end, | ||
}), | ||
} | ||
} | ||
|
||
return { | ||
state: newState, | ||
effect: Task.listen({ | ||
link: Task.effects(tasks), | ||
block: writeBlock(state.writer, block), | ||
fileLink: writeFileLink(state.unixFsFileLinkWriter, link), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!state.unixFsFileLinkWriter) { | |
return { | |
state: newState, | |
effect: Task.listen({ | |
link: Task.effects(tasks), | |
block: writeBlock(state.writer, block), | |
end, | |
}), | |
} | |
} | |
return { | |
state: newState, | |
effect: Task.listen({ | |
link: Task.effects(tasks), | |
block: writeBlock(state.writer, block), | |
fileLink: writeFileLink(state.unixFsFileLinkWriter, link), | |
return { | |
state: newState, | |
effect: Task.listen({ | |
link: Task.effects(tasks), | |
block: writeBlock(state.writer, block), | |
fileLink: state.unixFsFileLinkWriter ? writeFileLink(state.unixFsFileLinkWriter, link) : undefined, |
Nit: This feels more straightforward to me personally.
P.S.: See my other more comments that make this remark obsolete
// Set up the file writer with link metadata writer | ||
const unixFsFileLinkWriter = writable.getWriter() | ||
|
||
const fileWriter = createFileWriter({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally suggest different API design. Specifically instead of passing another stream to write links to I would simply defined something like
interface FileFragment extends Block {
link: UnixFS.FileLink
}
That way current consumer can continue using API as is while consumers interested in links could use .link
to get it. Also streams have .tee()
method allowing to have two different consumers if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of regret not just making stream EncodedFile
in which case we would not have this problem in first place, but perhaps switching to that as breaking change could be another way this could be introduced. You could still have something like createFileBlockWriter
for old code and new code could use createFileWriter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually thinking more about it I think it would be best to:
- Add
createEncodedFileWriter
that takesWriter<EncodedFile>
. - Create
createFileBlockWriter
that basically usescreateEncodedFileWriter
and maps values to.block
. - Create alias for
createFileWriter
with a deprecation messages suggesting to use one of the above two
I think this would create good migration path without having to introduce additional code paths or complexity.
Feature: Add Support for Optional UnixFsFileLinkWriter in createFile
This PR adds support for passing an optional UnixFsFileLinkWriter to the createFile function, enabling consumers to extract structured FileLink metadata while writing UnixFS files (and eventually do not require data transformation to serve content addressable data).
What’s New
Why This Matter
Usage snippet