-
Notifications
You must be signed in to change notification settings - Fork 324
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
Replace regexes with proper IPFS path/multihash validation #47
Comments
I've been updating on ipfs-chrome-station and ended up publishing |
A quick brain dump on this. Reusing For some background, a quote from this discussion:
In short, even if a library does not use Node-specific APIs, module resolution is broken in subtle ways: const isIPFS = require('is-ipfs') ends with:
I did some experiments and relative paths work fine, for example: const isIPFS = require('../node_modules/is-ipfs/index.js') produces a new error this time for its dependency:
Ok, it should be possible to go over every dependency and fix require paths via |
They say here that it just work: https://developer.mozilla.org/en-US/Add-ons/SDK/Tutorials/Using_third-party_modules_%28jpm%29 |
It "just works" for a limited number of modules that are tagged with jetpack. This mess is one of reasons why WebExtensions are developed as an alternative to current addon ecosystem. |
Ok, deep into a rabbit hole: CommonJS implementation in Firefox is unable to load const isIPFS = require('../node_modules/is-ipfs')
const b58 = require('../node_modules/bs58/index.js')
const baseX = require('../node_modules/base-x/index.js')
const multihashes = require('../node_modules/multihashes/dist/multihashes.js')
At this point I stopped. It does not seem to work for anything bigger than 'hello world' and there is a multitude of open tickets for NPM support:
It's just broken and I don't think it will be fixed for old SDK – all development goes into WebExtensions these days. It will probably be solved there. I've read that alternative way to run NPM packages in Firefox is via Browserify shims.
const isIPFS = require('./is-ipfs.js')
isIPFS.multihash('QmYjtig7VJQ6XsnUjqqJvj7QaMcCAwtrgNdahSiFofrE7o')
There are two downsides:
I think I'll give it a try, following the feedback from linked example (avoiding redundant shims etc). |
Ok, so Firefox SDK provides some Node-compatible libraries, but under different names. +"jetpack": {
+ "overrides": {
+ "buffer": "sdk/io/buffer"
+ }
+ }, Having that, I was able to hot-swap the implementation and remove all shims:
Produced:
This contains only multihash-related code and should be easier to review 🎈 |
Oh, sorry I wasn't following this thread, but glad that you got it working :) Do you think we should publish the browserified version too on |
No, npm package is enough. I have it as a dependency in I'll leave it this way as a workaround until Firefox gets proper npm package support in new API called WebExtensions (#20). |
As suggested in ipfs/notes#92 (comment) and #43 we should stop using regexes for detecting IPFS paths and come up with something more future-proof that involves js-multiaddr and js-multihash.
First step would be to create internal PoC in
lib/ipfs-path-validation.js
withisValidIPFSPath(path)
method and then refactor addon to use it instead ofgateways.IPFS_RESOURCE
andprotocols.IPFS_PATH
regexes.The text was updated successfully, but these errors were encountered: