Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: repo setup, adds tag and file api wrapper #1

Merged
merged 5 commits into from
Nov 19, 2020

Conversation

vojtechsimetka
Copy link
Contributor

@vojtechsimetka vojtechsimetka commented Nov 7, 2020

Features

  • basic setup of the repo including readme, code and commit lint
  • wrappers for tag and file api with tests
  • github actions CI spawning Bee cluster with 5 nodes, runs linter and nodejs test against nodejs v10, 12 & 14 with node_modules caching (thanks @vandot for the help)

TODO:

  • error handling (please indicate to which extend I should go here)
  • CI running browser tests in webworker
  • bind the actions to the bee class (suggestions how to do it best welcomed)
  • fix the utils/data.ts and utils/data.browser.ts substitution for something more sensible

@agazso
Copy link
Member

agazso commented Nov 9, 2020

I am not familiar with tasegir but it seems like to be a tool doing many things (setting up tests, managing versions, run the doc generator, add Travis CI support) in an opinionated way. Personally I prefer to keep things lean and use tools when they are needed, but I can be convinced that this is something that helps us in the long term. What I would like to avoid is that because we use a tool such like this we are bound to use only those tools that it supports.

@@ -0,0 +1,47 @@
**[@ethersphere/bee-js](../README.md)**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commiting generated files in a repo may not be a best practice in general, because it's possible that the source and the generated files are out of sync, it generates lot of noise etc.

Let's discuss if there is a good reason to do this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the .gitignore is still missing, so every generated files coould be pushed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it is not good idea to have the generated docs in the repo but I figured for the time being (before we have external docs) it's OK.

Fixed the .gitignore missing though, sorry for that

export default class Bee {
public readonly url: string

constructor (url: string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Typescript it's possible to write this in a shorter form:

constructor(readonly url: string) {}

Then you don't need neither the variable declaration in the class, neither the assignment in the constructor body. Let's discuss if we want to use this kind of idiom (called parameter properties)

options?: OptionsUpload
): Promise<string> {
return (
await axios({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All axios calls could be wrapped in a function that checks if there were errors. Then returning .data would be safe (it could even be made typesafe :) and all errors could be returned in a well-defined and documented way as well.

For example this current function can either throw an exception from axios code, or return undefined (in case of reference is not defined on data) or return the reference itself, which happens to be a string returned by Bee but that's a coincidence here, because you expect 'application/octet-stream` so it could be even a random byte array that would return garbage string data.

const BEE_URL: string = process.env.BEE_URL || 'http://bee-0.localhost'

describe('Bee class', () => {
let bee: Bee
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest considering using immutable classes. One benefit of that is easier testing and not having to use let at all :)

In this case that would mean we could have only one Bee instance set up and we wouldn't need a before block. It doesn't seem like a huge win here, but in general it makes tests easier to understand if they are stateless in my experience.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I would utilize this practice in every case, the reference object initializations should happen with const keyword, except well justified cases, for example on browser or node backed service initialization according to the running environment.

bee = new Bee(BEE_URL)
})
it('should give proper bee URL', () => {
expect(bee.url).to.equal(BEE_URL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like to be a very useful test to me :)

In general tests are supposed to test when there is a logical condition in the code where depending on the inputs the flow of the program changes. Here it is not case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the Bee class doesn't have any functionality which could include the already existing wrapper API methods, and so after it could be tested with these functions if it integrates well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll remove this test. I just needed something to test the github actions and forgot to remove.

const tag = await Tag.createTag(`${BEE_URL}/tags`)
await File.upload(`${BEE_URL}/files`, file, { tag: tag.uid })

await sleep(2000)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sleep and other time related functions are usually considered a red flag in tests. They make tests non-deterministic and may cause errors that are hard to reproduce and fix (see flaky tests).

In this case this seems to be an integration test that are waiting for an output from an external process. In that case it may be justifiable but then it's better to check repeatedly until a timeout. For that purpose we might define our own test helper functions.

*
* @param length
*/
export function randomBuffer (length: number): Buffer {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another problem with this random function (besides the safety issue that you correctly pointed out) is that it's not deterministic. This makes it problematic when using it in tests because every test run will be unique and unreproducible.

Usually it's a better approach to use a hardcoded random seed and using a pseudo-random generator seeded with that number. That way tests become reproducible.

@nugaon nugaon merged commit d1d1484 into master Nov 19, 2020
@agazso agazso mentioned this pull request Jan 11, 2021
7 tasks
vojtechsimetka referenced this pull request in vojtechsimetka/bee-js Jan 14, 2021
* feat: added browser target to the release bundle
* ci: create release branch when merged to master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants