Skip to content
This repository was archived by the owner on Feb 12, 2024. It is now read-only.

Support web File's lastModified field #3187

Closed
Gozala opened this issue Jul 20, 2020 · 3 comments
Closed

Support web File's lastModified field #3187

Gozala opened this issue Jul 20, 2020 · 3 comments
Assignees

Comments

@Gozala
Copy link
Contributor

Gozala commented Jul 20, 2020

This is the work that I've decided to take out of #3151 in order to reduce it's scope & something that can be done as a followup.

Issue

Following example illustrates user providing "mtime" explicitly, which current implementation will ignore by including mtime information.

ipfs.add(new File(['hello', 'world'], 'hello.txt', { lastModified: 1595277030533 })

On the other hand following code would include mtime:

ipfs.add({ content: 'hello world', name: 'hello.txt', mtime: new Date(1595277030533) })

Rational for not deriving mtime from lastModified is following:

  1. All the dragged files will have those and ipfs add /path/to/file.txt (in CLI) does not include mtime either.
  2. Doing ipfs.add(new File(content, name)) does not explicitly provide lastModified but it is still there.

Plan

After discussing this with @achingbrain we have came to conclusion that best compromise would be to ignore lastModified unless user explicitly opts-in (similar to how ipfs add accepts --preserve-mtime flag). In other words following would not include mtime:

ipfs.add({ content: 'hello world', name: 'hello.txt', mtime: new Date(1595277030533) })

But user could override that default using preserveLastModified option:

ipfs.add({ content: 'hello world', name: 'hello.txt', mtime: new Date(1595277030533) }, {
  preserveLastModified: true
})

Option preserveLastModified has no effect on inputs different from File. Same option will be added to ipfs.addAll and have exact same effect. If set to true all File instances will have their lastModified info preserved via mtime otherwise they are going to be ignored.

@Gozala Gozala added the need/triage Needs initial labeling and prioritization label Jul 20, 2020
@Gozala Gozala self-assigned this Jul 20, 2020
@achingbrain
Copy link
Member

The only thing that gives me pause here is that the proposal adds a special option just for browser File objects.

Where we've needed special source handling before we've added source converters like globSource and urlSource which set options or convert things to the required types.

We could add a fileSource here that takes the preserveLastModified option? That way we can keep it out of core.

@achingbrain achingbrain added exploration and removed need/triage Needs initial labeling and prioritization labels Jul 22, 2020
@Gozala
Copy link
Contributor Author

Gozala commented Jul 23, 2020

I personally was hoping that native File would be something we could embrace over time instead of custom duck typed file objects. #3151 was pushing towards that direction and I was thinking that node File polyfill could be an extension of fs.ReadStream.

I'm not big fan of urlSource honestly because with improvements to normaliseInput one could just as easy use fetch and pass a blob. And if we want to improve this further I'd consider adding Response to the list of supported inputs, instead.

We could add a fileSource here that takes the preserveLastModified option? That way we can keep it out of core.

Alternatively we could have have a more general saveMTime options that works across all of the input types. I'd also point out that browser specific options are going to creep up e.g. shared worker API add's transfer options pretty across the board so that user can provide references to buffers that can be transferred vs copied.

@Gozala
Copy link
Contributor Author

Gozala commented Aug 6, 2020

We've decided not to do this.

@Gozala Gozala closed this as completed Aug 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants