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

feat: Poor Man's Protocol Handlers #283

Merged
merged 8 commits into from
Sep 27, 2017
Merged

Conversation

lidel
Copy link
Member

@lidel lidel commented Sep 17, 2017

This is an implementation of ridiculous idea from #164 (comment)
I am really sorry, but its the only way to get dweb:// working across all browsers 🙄


As usual, I am not sure how to label checkbox responsible for toggling this feature in Preferences.
Current label looks like this:

2017-09-17-024645_537x81_scrot

Any ideas how to improve it before I merge this PR?

This is an implementation of ridiculous idea from:
#164 (comment)

I am really sorry.
@lidel lidel added this to the v2.0.0 milestone Sep 17, 2017
@lidel lidel self-assigned this Sep 17, 2017
@lidel lidel requested review from daviddias and Kubuxu September 17, 2017 00:59
@ghost
Copy link

ghost commented Sep 17, 2017

Awesome :):) Just from a quick glance it might be better to remove dweb:// here since it's actually dweb: and a URI scheme, not a URL scheme. Or can you handle URIs here too? Then dweb:/ipfs/$cid and dweb:/ipns/$peerid would be correct examples of these.

@lidel
Copy link
Member Author

lidel commented Sep 17, 2017

Good question @lgierth !
Right now it supports both single and double slash ( dweb:/ipfs/$cid and dweb://ipfs/$cid )
Should it also support versions without leading slash (so that ipfs:$cid works too) ?

@ghost
Copy link

ghost commented Sep 17, 2017

Right, you get the whole string so you can match whatever you want :) It should only be ipfs:// ipns:// and dweb:. I've promised to write exactly this down for a while (sorry @diasdavid!)

Should it also support versions without leading slash (so that ipfs:$cid works too) ?

This would not be a URL anymore, we'd then have a URL scheme named ipfs and a URI scheme named ipfs

@ghost
Copy link

ghost commented Sep 17, 2017

It should only be ipfs:// ipns:// and dweb:

And maybe later also ipld:// if we decide for an /ipld namespace (I think that's still up for discussion?)

@ghost
Copy link

ghost commented Sep 17, 2017

For reference ipfs/specs#152 (comment)

@lidel
Copy link
Member Author

lidel commented Sep 17, 2017

Thanks, I've updated label to:

2017-09-17-163656_519x100_scrot

That being said, extension is more liberal at the moment and both :/ and :// work fine for all schemes (requests are normalized to HTTP URL and redirected to public/local gateway).

Is that okay, or should we explicitly only support ipfs://$cid, ipns://$peerid and dweb:/ipfs/$cid?

Should fs: be removed?

@ghost
Copy link

ghost commented Sep 17, 2017

Is that okay, or should we explicitly only support ipfs://$cid, ipns://$peerid and dweb:/ipfs/$cid?

Better only support what's explicitly supported. Otherwise someone will start using it and we eventually 1) break people's stuff when we remove that implicit support, or 2) have to actually continue supporting it.

Should fs: be removed?

Yeah can drop fs:

- Support `dweb:` address scheme: closes #280
- Disabled support for unsupported schemes, as described in
  #283 (comment)
- Improve performance on complex and dynamic pages: closes #231
- Notifications always go to Browser Console
- It is possible to disable notification popups: closes #282
- Updated some dependencies
@lidel lidel merged commit 7d3a5e0 into master Sep 27, 2017
lidel added a commit that referenced this pull request Sep 27, 2017
lidel added a commit that referenced this pull request Sep 27, 2017
lidel added a commit that referenced this pull request Sep 27, 2017
- Support `dweb:` address scheme: closes #280
- Disabled support for unsupported schemes, as described in
  #283 (comment)
- Improve performance on complex and dynamic pages: closes #231
@lidel lidel deleted the poor-mans-protocol-handlers branch September 27, 2017 23:13
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.

1 participant