search

plebbit / plebbit-js

A Javascript API to build applications using plebbit
GNU General Public License v2.0
41 stars 7 forks source link

Define PlebbitOptions required for tests #11

Closed estebanabaroa closed 1 year ago

estebanabaroa commented 1 year ago

Some options are required for tests that use plebbit-js outside of plebbit-js, like plebbit-react-hooks. Options such as using a Memory DB, subplebbit/comment update time, subplebbit publish interval time, etc.

A list should be defined in this issue and then added to the readme and become public APIs.

To start I suggest:

interface PlebbitOptions {
  publishInterval?: number // in ms, the time to wait for subplebbit instances to publish updates
  updateInterval?: number // in ms, the time to wait for comment/subplebbit instances to check for updates
  noData?: boolean // if true, dataPath is ignored, all database and cache data is saved in memory
}

Note: ENV vars should not be used for this as they can be used to fix a not working CI in one repo, but wont fix it in another repo.

Rinse12 commented 1 year ago

That looks good to me. Although we should add an extra option for publishing to an offline ipfs client.

In ipfs-http-client, you need to enable allowOffline when you call client.publish, right now it's true by default, but it should be an actual documented option.

estebanabaroa commented 1 year ago

That looks good to me. Although we should add an extra option for publishing to an offline ipfs client.

Great you should implement it whenever you have time. You should also stop using the non public APIs in tests that do the same thing so the new APIs get tested.

In ipfs-http-client, you need to enable allowOffline when you call client.publish, right now it's true by default, but it should be an actual documented option.

Can't we just let allowOffline on all the time? Why would the user not want to allowOffline?

Rinse12 commented 1 year ago

Can't we just let allowOffline on all the time? Why would the user not want to allowOffline?

We could, but it's always better to be explicit about it. The user might be connecting to an offline node and publishing to it without realizing.

estebanabaroa commented 1 year ago

Can't we just let allowOffline on all the time? Why would the user not want to allowOffline?

The user might be connecting to an offline node and publishing to it without realizing.

I can't think of a real life scenario where this would cause any user issue. If the user has set up his node to be offline and tries to use plebbit with it, he will encounter many other problems, having this variable enabled won't change much imo. It's up to him to know his node configuration. There's also countless other configuration that would prevent his node from working, and we can't protect against those.

We could, but it's always better to be explicit about it.

Imo the priority is to have the least amount of APIs and options to maintain as possible. Being explicit is not a priority imo. We should only add options that are absolutely required in order for plebbit to work.

Let's say that there was another IPFS API that absolutely needed to know if the node is online/offline to work, maybe plebbitOptions.ipfsOffline?: boolean or maybe a more general plebbitOptions.test?: boolean could be added. But it doesn't seem to be absolutely needed for now, so we shouldn't add it.

Rinse12 commented 1 year ago

Should be implemented on commit #2e33610eeb278f35ce841e2c9a38fbc224e1ace2