feat: dynamic format loading #178
Conversation
This backwards compatible PR allows missing IPLD formats to be loaded dynamically. This follows on from the discussion here #164 (comment) A new constructor option `loadFormat` allows users to asynchronously load an IPLD format. The IPLD instance will call this function automatically when it is requested to resolve a format that it doesn't currently understand. This can re-enable lazy loading for IPLD modules in IPFS and also means that people who just want to add _more_ resolvers can do so without having to depend directly on the defaults and pass them in as well. License: MIT Signed-off-by: Alan Shaw <[email protected]>
ghost
assigned 
Codecov Report
@@ Coverage Diff @@
## master #178 +/- ##
=========================================
+ Coverage 89.05% 93.4% +4.34%
=========================================
Files 1 1
Lines 201 197 -4
=========================================
+ Hits 179 184 +5
+ Misses 22 13 -9
Continue to review full report at Codecov.
|
|
|
||
| ```js | ||
| const ipld = new Ipld({ | ||
| loadFormat (codec, callback) { |
There was a problem hiding this comment.
Wow, I didn't know that this syntax is a things. It's nice to learn new things by code reviewing.
| }) | ||
|
|
||
| it('should fail to dynamically load format via loadFormat option', (done) => { | ||
| const errMsg = 'BOOM' + Date.now() |
There was a problem hiding this comment.
Why the Date.now()? I generally prefer tests that do the exact same across runs.
There was a problem hiding this comment.
In this case I want to ensure the error that was thrown was the error that I threw. I'm asserting on the error message so I need a value that is not going to cause a false positive with any other error that could be thrown.
So, yes, I guess I could just use a static string but there's a tiny bit more risk of it causing a false possitive in the future than a value that is randomly generated.
In general I know that tests are not isolated from side effects, so I like to know my tests are dealing with their own unique data. As an example we share block stores across multiple tests and they don't clean up after themselves. By the way, I don't believe they should clean up and we can't guarantee that it'll happen (in the case of failures) or that unintentional side effects don't happen. Ideally we'd use a new block store for every test, but I appreciate that's not always practical....The point is, using randomly generated data is one way of better isolating tests from side effects caused by other tests.
A completely contrived example: I'm testing I can put a value with IPLD and I use a static value. Many weeks later someone adds a test before mine that puts that same value in the block store. Since the block store is shared, when my test runs the code somehow determines the value is already in the store and returns success. Same result, but it's a different code path that's being tested now, and maybe that's masking a error with the code path that writes to disk...
There was a problem hiding this comment.
BTW, I'm happy to change if you disagree or would prefer!
There was a problem hiding this comment.
Thanks for the explanation. It makes sense, happy to keep it that way.
| const bs = new BlockService(repo) | ||
| const resolver = new IPLDResolver({ blockService: bs }) | ||
|
|
||
| data = { now: Date.now() } |
There was a problem hiding this comment.
Why the Date.now()? I generally prefer tests that do the exact same across runs.
|
|
||
| const IPLDResolver = require('../src') | ||
|
|
||
| module.exports = (repo) => { |
There was a problem hiding this comment.
Would it make sense to add a test that a format is not dynamically loaded when it was already statically added?
License: MIT Signed-off-by: Alan Shaw <[email protected]>
This PR uses the new `loadFormat` option for IPLD to lazily require IPLD formats in order to reduce the startup time for the node. If you're feeling like you've seen this before then, for reference: The PR ipld/js-ipld#164 undid the work done in ipld/js-ipld#145 and ipld/js-ipld#178 re-enabled lazy loading. License: MIT Signed-off-by: Alan Shaw <[email protected]>
This PR uses the new `loadFormat` option for IPLD to lazily require IPLD formats in order to reduce the startup time for the node. If you're feeling like you've seen this before then, for reference: The PR ipld/js-ipld#164 undid the work done in ipld/js-ipld#145 and ipld/js-ipld#178 re-enabled the ability to do so. This PR makes use of this new ability to lazy load the formats. License: MIT Signed-off-by: Alan Shaw <[email protected]>
This backwards compatible PR allows missing IPLD formats to be loaded dynamically.
This follows on from the discussion here #164 (comment)
A new constructor option
loadFormatallows users to asynchronously load an IPLD format. The IPLD instance will call this function automatically when it is requested to resolve a format that it doesn't currently understand.This can re-enable lazy loading for IPLD modules in IPFS and also means that people who just want to add more resolvers can do so without having to depend directly on the defaults and pass them in as well.
It also removes a bunch of repeated "No resolver found for codec" blocks so the code is more DRYFTW \o/