refactor: remove proxy api object and detect initialisation state #2762
Conversation
|
Nice! I came to the same conclusion as I was filling out the rest of the API (after the initial PR to add the api manager) i.e. it's better to just be explicit about the API exposed at each stage of the lifecycle rather than just filling in the bits that you want to expose and relying on the proxy to throw for APIs that are not defined - I just never got round to going back and sorting it. Thanks for tidying the mess 😅 |
achingbrain
changed the title
| }) | ||
|
|
||
| async function create (options) { | ||
| options = mergeOptions(getDefaultOptions(), options) | ||
|
|
||
| // eslint-disable-next-line no-console | ||
| const print = options.silent ? log : console.log | ||
| const repo = typeof options.repo === 'string' || options.repo == null | ||
| ? createRepo({ path: options.repo, autoMigrate: options.repoAutoMigrate }) | ||
| : options.repo |
There was a problem hiding this comment.
I don't understand why repo has to be created here now instead of in init.js?
It means we can't pass a repo option to ipfs.init({ repo }) anymore...
There was a problem hiding this comment.
The idea was you'd ask the repo if it was initialised or not and if so allow the user to call .start() without having to call .init() first.
That being said, the only way to get the .start() method into the API is to call .init() as .init() not only might initialise your node but also determines the next state the it can be in by changing the available API methods.
TBH I'm not sure why we have a separate .init() method any more - an instantiated but not initialised IPFS node is not a lot of use to anyone. It seems like a hangover from when this module exported a constructor but we still needed to do some async work to set up the node so someone thought an initialiser would be a good idea. Now that we have IPFS.create() as the preferred method of creating a node we don't really have that restriction any more.
Given that the cli now auto-inits when you start the daemon maybe we have precedent to remove it?
There was a problem hiding this comment.
Remove init method would mean remove the init option and create always inits? If init fails create throws? If so we wouldnt need the API manager it's just matter of connectivity either a method needs it or not to function.
Do we have everything related to opening/using a repo separated from initializing (creating a new repo) ?
I also think it was nice to export a constructor plus a static factory like create but I guess there's small value in that.
There was a problem hiding this comment.
If so we wouldnt need the API manager it's just matter of connectivity either a method needs it or not to function.
The swapping out of API methods that are only available online/offline is neat so I wouldn't necessarily get rid of the API manager. It means we don't need loads of repeated isOnline() checks and error throwing.
Do we have everything related to opening/using a repo separated from initializing (creating a new repo) ?
That what this PR sort of does, but the logic is still a bit mixed up inside .init() and could use some more untangling.
I also think it was nice to export a constructor plus a static factory like create but I guess there's small value in that.
In principal an object should be complete (e.g. usable) after construction, in JS we can't always do this in a constructor because sometimes we need to do some async work and constructors must be synchronous (discounting weird contortions) so I think exporting a constructor in our case should be considered harmful.
Instances of IPFS are definitely not usable before .init() has been called.
There was a problem hiding this comment.
The idea was you'd ask the repo if it was initialised or not and if so allow the user to call
.start()without having to call.init()first.
Gotcha. I'm not convinced it's worth pulling it out of init() for that, given that the init option defaults to true so most people do not have to call it before start anyway. If I explicitly set the init option to false I don't think it's completely unreasonable to expect to have to call init() before start().
I prefer the idea of not having init() and having the steps init() performs done in create(). The init function is probably there historically because ipfs init is part of the CLI https://docs.ipfs.io/reference/api/cli/#ipfs-init
So to get this straight the proposal would be to remove the ipfs.init() function, having IPFS.create() perform these steps. If you don't want to init, then don't call IPFS.create().
The constructor option init passed as a boolean would be invalid, but we'd still want to allow passing an object for repo initialization options like here.
To answer the comment here, allowNew is currently used when using the CLI without a daemon running. It prevents you from trying to use the API without initializing a repo first. I think we should keep this behaviour because I don't believe that using API methods that aren't init/daemon should have the side effect of creating a repo somewhere on your hard drive.
There was a problem hiding this comment.
Sounds good, I'll make the changes to .create.
I don't believe that using API methods that aren't init/daemon should have the side effect of creating a repo somewhere on your hard drive.
That seems fair enough, but which CLI operations can you use with a node but without a repo? I can only see things like ipfs version, ipfs commands and ipfs --help and we shouldn't be creating a node for them.
The only thing we can do is throw a not initialized error, and the only thing the user can do then is to initialise the repo and run their command again so we might as well cut out the middle man and do it for them.
Otherwise pretty much the first thing a user sees is an error:
$ npm install -g ipfs
$ jsipfs add ./some-file.txt
Error: Not initializedIndeed, the user can do ipfs --help and see a whole bunch of commands, most of which will explode if they try to use them.
There was a problem hiding this comment.
That error message needs to be much better! I'm sure that used to be better and that's a regression.
I think that it's valuable to guard against adding content to a repo that doesn't exist, and I don't think the commands should all have a side effect of creating one like this. I'd also rather the output from each one be predictable - you'll get initialization messages at the moment like this if a repo is automatically initialized.
I take the point of removing a hurdle, but I do feel it is justified here and it's a one time thing anyway, and only if the user does skip straight to ipfs add instead of following the guide.
There was a problem hiding this comment.
What if we do the init() removal work in a separate PR? We don't have time to get this into 0.41 now, but there's lots of good work here that could go out in a patch release.
Ask the repo if it has been initialised, if so, allow the user to
skip the `.init()` step and move on to `.start()`
Removes the proxy api object in favour of vanilla functions because
it was causing errors to be thrown if you even referenced properties
that were from a different api state.
E.g. with an unitialised repo:
```javascript
const ipfs = await IPFS.create({
init: false,
start: false
})
// no invocation, just referencing the property causes an error to be thrown
console.info(ipfs.start)
```
I'd looked at changing the proxy behaviour to return a function that
throws if invoked, but at the time the proxy is called you don't know
what the calling code is going to do with the return value so it's hard
to know if it's accessing a function or a property - the return value is
just put on the stack and interacted with so it seemed simpler to just
pull it out and define the API up front.
A nice future improvement might be to have `.init`, `.start` and `.stop`
export functions that update the API - that way after `.stop` has been
invoked, it could restore the API from the post-`.init` state, but this
can come later.
Also upgrades `ipfsd-ctl` to pass refs only during factory creation.
Depends on:
- [ ] ipfs/js-ipfsd-ctl#457
- [ ] ipfs/js-ipfs-repo#219
- [ ] ipfs-inactive/npm-go-ipfs-dep#40
fix: do not allow parallel init, start or stop
If the user is calling `.init`, `.start` or `.stop` from the code
in multiple places simultaneously, they probably have a bug in
their code and we should let them know instead of masking it.
achingbrain
force-pushed
the
allow-user-to-skip-init-when-repo-initted
branch
from
b4c4478
to
7f3a2d7
Compare
Ask the repo if it has been initialised, if so, allow the user to skip the
.init()step and move on to.start()Removes the proxy api object in favour of vanilla functions because it was causing errors to be thrown if you even referenced properties that were from a different api state.
E.g. with an unitialised repo:
I'd looked at changing the proxy behaviour to return a function that throws if invoked, but at the time the proxy is called you don't know what the calling code is going to do with the return value so it's hard to know if it's accessing a function or a property - the return value is just put on the stack and interacted with so it seemed simpler to just pull it out and define the API up front.
A nice future improvement might be to have
.init,.startand.stopexport functions that update the API - that way after.stophas been invoked, it could use that function to restore the API from the post-.initstate, but this can come later.Also upgrades
ipfsd-ctlto pass refs only during factory creation.Depends on: