Closed capellini closed 8 years ago
@gr2m -- it looks like the first step, as mentioned by @espy, is to revise getState
so that the options are not ignored. This first test just tests for the account
property, but I plan to add others as I get a better understanding of the implementation. My initial thoughts on implementation are to do something like this in getState
:
return merge(defaultOptions, options, requiredProperties)
Where defaultOptions
are options that have default values, but may be overridden by properties in options
(e.g. url
and emitter
), options
are the options that are passed into the constructor by the user, and requiredProperties
are those that are pulled from configuration and set by getState
(e.g. id
).
What do you think of this as a first step?
I can see some potential problems in just merging in options
as-is -- since it's user input, should we trust it? The requiredParameters
will be overridden in the merge
, but are there other ways that we can get in trouble by just blindly accepting user input in this way?
Also, according to the docs, the only options that are currently accepted are url
, account
, store
, task
, and connectionStatus
. Therefore, should I filter out all other properties from options
before performing the merge
with defaultOptions
and requiredProperties
? I suppose that this would be the most 'locked-down' way to do things, but may also make the code brittle, as it would require changes every time that we add or remove a configurable option property.
you can use .defaultsDeep to use default values and then overwrite them with the passed options.
I’d pass on all options by the user, because soon we want to support plugins, and then we want to be able to pass in plugin-related options, too
@gr2m -- This latest commit will allow a user to pass in options to getState
. It looks like the next step is to continue to pass those options into getApi
. I'm not all that familiar with hoodie.account
. If I pass in options to the account constructor in getApi, will the account accept the arguments, or would we also have to modify hoodie-account-client
to accept and use options passed by the user in the constructor?
Also, I should probably create a tests/specs/get-api.js
file for this PR, since we'll most likely have to create many tests for the various options that we'll be passing into getApi
now, right? Or would you rather I put the tests somewhere else?
Thanks for another great pull request, @capellini 👍
I'm not all that familiar with hoodie.account. If I pass in options to the account constructor in getApi, will the account accept the arguments, or would we also have to modify hoodie-account-client to accept and use options passed by the user in the constructor?
Good point, right now we only pass the url
option to it and don’t pass any options from the Hoodie Client constructor.
The right thing to do here would be to take options.account
if it’s set, set url
on it as it is always the same for Hoodie and cannot change, and leave the rest. For example if we do
new Hoodie({
account: {
validate: validateAccountFunction
}
})
then internally the Account
constructor should be called with {url, validate}
.
It is the same for ConnectionStatus
and Log
. The Store Client does not accept any other configuration right now so we can ignore it at the moment.
Can you put that in, too? It would be great if you could add tests to tests/specs/init.js to test that options get passed correctly
Will do.
My checklist:
@gr2m -- should be good to go. The tests that I created are definitely unit tests -- they only test that the options passed in are correctly passed into the respective constructor. Do you think any integration tests are necessary?
Also, if all looks good, feel free to send more work my way. Looking forward to your feedback!
This looks great, thanks! LGTM ping @hoodiehq/maintainers
If you like, I recently suggested to merge the 3 repositories hoodie-store-client
, pouchdb-hoodie-api
and pouchdb-hoodie-sync
in https://github.com/hoodiehq/hoodie-store-client/issues/125
There was no opposition within a week, so I think we are good to go. This will help us to reduce the differences in behavior between hoodie.store.*
and hoodie.store(scope).*
APIs. For example https://github.com/hoodiehq/hoodie-store-client/issues/95 & https://github.com/hoodiehq/hoodie-store-client/issues/89 would probably get resolved by this.
This would be a bigger project, but I’m happy to guide you trough
LGTM
If you like, I recently suggested to merge the 3 repositories hoodie-store-client, pouchdb-hoodie-api and pouchdb-hoodie-sync in hoodiehq/hoodie-store-client#125
There was no opposition within a week, so I think we are good to go. This will help us to reduce the differences in behavior between hoodie.store.* and hoodie.store(scope).* APIs. For example hoodiehq/hoodie-store-client#95 & hoodiehq/hoodie-store-client#89 would probably get resolved by this.
This would be a bigger project, but I’m happy to guide you trough
@gr2m -- sounds good to me. Looking forward to contributing on a larger hoodie project like this. I'll start looking around the three repositories. I imagine that we'll be creating a new repository for the project?
@capellini I missed something with my idea to combine the three: https://github.com/hoodiehq/hoodie-store-client/issues/125#issuecomment-246126156
I think it makes sense for use to at least keep hoodie-store-api separated. But still I think it’s worth to look into how hoodie-store-api
and how the add/find/update/remove APIs work in hoodie-store-client
, and how we can avoid code duplication and can maybe reuse tests or error handling etc.
Currently, the docs say that we can pass options into the hoodie constructor, but the options are completely ignored. This change allows users to pass an options object into the Hoodie constructor in which the constructor options mentioned in the docs are not ignored.
Closes hoodiehq/hoodie-client#89.