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

Add gateway route /ipfs/* #698

Merged
merged 6 commits into from
Aug 29, 2017
Merged

Add gateway route /ipfs/* #698

merged 6 commits into from
Aug 29, 2017

Conversation

harshjv
Copy link
Contributor

@harshjv harshjv commented Jan 3, 2017

Add gateway route /ipfs/*

  • Stream content with proper content-type when file name is available
  • Allow directory listing with the same style as https://ipfs.io/ipfs/<some-dir-hash>

Preview

screen shot 2017-01-03 at 7 15 02 pm

Related issue

#693

@daviddias daviddias added the status/deferred Conscious decision to pause or backlog label Jan 11, 2017
Copy link
Contributor

@kumavis kumavis left a comment

Choose a reason for hiding this comment

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

Looks good. Did not actually run or test it. There's some minor things.

  1. non-standard white-spacing ( aligning against token on previous line instead of just depth * indentSize )
  2. introduces some TODO's in comments -- better to leave them out, note whatever shortcomming, and create an issue for it.

I guess I'd also expect go-ipfs and js-ipfs to just serve the same app, so it can be iterated on as a separate project

@kumavis
Copy link
Contributor

kumavis commented Feb 13, 2017

test fails bc of some gulp-coverage breakage

@harshjv
Copy link
Contributor Author

harshjv commented Feb 15, 2017

@kumavis Fixed indentation and removed TODO comment

@kumavis
Copy link
Contributor

kumavis commented Feb 15, 2017

Dont mind the security warning, its a known situation and is in an outdated copy of the build tool

@kumavis
Copy link
Contributor

kumavis commented Feb 15, 2017

I guess I'd also expect go-ipfs and js-ipfs to just serve the same app, so it can be iterated on as a separate project

@diasdavid thoughts?

otherwise LGTM

@daviddias
Copy link
Member

daviddias commented Feb 16, 2017

I won't have the time to review this soon. Nevertheless one thing that caught my eye is that there aren't any tests. Also, if we are going to push forward and add the gateway to js-ipfs, I would prefer to:

@dignifiedquire
Copy link
Member

One other note, the current code base does not use promises, and we would like to keep it that way for now.

@kumavis
Copy link
Contributor

kumavis commented Feb 16, 2017

One other note, the current code base does not use promises, and we would like to keep it that way for now.

I think this is another point for breaking the ui out into its own project

@daviddias
Copy link
Member

Yeah, there is the webui and this 'file viewer' thing. Both can be their own projects I believe, transforming the file viewer to a NWA instead of a server side rendered thing, so that it can be used easily by go and js-ipfs.

@victorb
Copy link
Member

victorb commented Mar 21, 2017

@diasdavid bit unclear what the decision is for this PR. Should we focus on merging this or should we focus on splitting it out and integrate as external module?

@daviddias
Copy link
Member

@victorbjelkholm

@harshjv
Copy link
Contributor Author

harshjv commented Mar 26, 2017

@daviddias
Copy link
Member

daviddias commented Jul 7, 2017

@harshjv completed missed the last comment. https://github.com/harshjv/ipfb looks great, that file browser should be something that both go and js can use and not only js.

Could you handle these tasks first:

I would be comfortable merging this if these tasks were completed. We can figure out how to have 'one file-browser' for both implementations next. It is very probable that by having the Gateway working on js-ipfs, that we will develop an even better file browser since it will enable more JavaScripters to play with it :)

Btw, js-ipfs now fully supports paths as in /ipfs/hash/a/b/c.txt

@ya7ya
Copy link
Contributor

ya7ya commented Aug 22, 2017

@diasdavid @harshjv Hi there, Are there any plans to fix the minor conflict and merge this PR ??

@daviddias
Copy link
Member

@ya7ya see above, there are more things to do than merge conflicts. Wanna help?

@ya7ya
Copy link
Contributor

ya7ya commented Aug 23, 2017

@diasdavid Yeah, I'm currently swamped with work trying to figure out the problem with #946 which is desperately needed for a project i'm working on, but I can try to help by finishing the tasks mentioned in your previous comment

@ya7ya
Copy link
Contributor

ya7ya commented Aug 24, 2017

hey @diasdavid , I been working on the points you mentioned above to get this thing done, But there is an issue I thought you might be able to clarify.

The go-ipfs implementation throws an error when you cat a directory, which is useful to figure out whether a path should be served or render the ui for the index. in js-ipfs this error is never triggered so figuring out the directory isn't straightforward.

Does js-ipfs cat implmentation has that ErrIsDir Error ?? or do I need to figure out another way to find that out ?

@ya7ya ya7ya mentioned this pull request Aug 28, 2017
4 tasks
@daviddias daviddias changed the base branch from master to feat/gateway August 29, 2017 07:21
@daviddias
Copy link
Member

Merging this PR into a new target branch feat/gateway so that @ya7ya and @harshjv can collaborate together on it (without having to PR the PRs)

@daviddias daviddias merged commit cc41624 into ipfs:feat/gateway Aug 29, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/deferred Conscious decision to pause or backlog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants