fix: only require http api client if it has not been specified#450
fix: only require http api client if it has not been specified#450hugomrdias merged 17 commits intomasterfrom
Conversation
achingbrain
force-pushed
the
only-require-http-client-if-not-specified
branch
2 times, most recently
from
2a6f984 to
d4082db
Compare
If we configure which http api module to use, don't try to require `ipfs-http-client`, because that then means the containing project needs to specify a dependency on `ipfs-http-client` when the containing project might actually be `ipfs-http-client`...
achingbrain
force-pushed
the
only-require-http-client-if-not-specified
branch
from
d4082db to
3a0f870
Compare
|
hummm why did this jumped +9kb in size |
|
Transitive dependencies changed? |
No idea why this is necessary
|
its 9kb gziped would need a lot random stuff from deps, can you run the aegir build analize please ? just to double check theses changes dont make us include http-client twice |
|
We shouldn't really be including I ran the analyser - it's quite hard to make out what's going on, they're definitely in there though only once that I can see. Before:
After:
I've added them to webpack's externals list so they don't get added to the bundle. Not sure if that's going to make the browser version explode at runtime or not. |
|
Lol:
|
|
its gonna break browser tests please remove the last commit and lets merge/release this. tip: use chrome to see the analyser, for some reason brave is broken. |
achingbrain
force-pushed
the
only-require-http-client-if-not-specified
branch
from
9970cf7 to
43e01d8
Compare
|
WAT! 🚀🐭 |
Co-Authored-By: Hugo Dias <hugomrdias@gmail.com>
src/factory.js
Outdated
| if (options.ipfsModule) { | ||
| delete opts.ipfsModule | ||
|
|
||
| if (options.ipfsModule.path) { | ||
| opts.ipfsModule = { | ||
| path: options.ipfsModule.path | ||
| // n.b. no ref property - do not send code refs over http | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (options.ipfsHttpModule) { | ||
| delete opts.ipfsHttpModule | ||
|
|
||
| if (options.ipfsHttpModule.path) { | ||
| opts.ipfsHttpModule = { | ||
| path: options.ipfsHttpModule.path | ||
| // n.b. no ref property - do not send code refs over http | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
why do we need all this haven't we already done this below? we just need to remove refs like in the previous version of this block.
Plus its deleting and setting opts.ipfsModule where it should be opts.json.ipfsModule and same for opts.ipfsHttpModule, so if this code made some use case work it wasn't because of this logic.
There was a problem hiding this comment.
It was resulting in ipfsModule: {} being sent over the http api which was then getting fleshed out to ipfsModule: { path: require.resolve('ipfs'), ref: require('ipfs') } which breaks under js-ipfs
There was a problem hiding this comment.
these if clauses are very hard to track and error prone because of all the combinations available in ctl
There was a problem hiding this comment.
Fair point about the deletion of the wrong property though.
There was a problem hiding this comment.
these if clauses are very hard to track and error prone because of all the combinations available in ctl
You are quite right. Here, look - I've removed them: #454
3 participants
If we configure which http api module to use, don't try to require
ipfs-http-client, because that then means the containing project needs to specify a dependency onipfs-http-clientwhen the containing project might actually beipfs-http-client...