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

Modernizing and using flowtype #60

Closed
wants to merge 6 commits into from
Closed

Modernizing and using flowtype #60

wants to merge 6 commits into from

Conversation

beaucollins
Copy link
Contributor

Builds off of #56

  • adds flow type
  • cleans up channel to remove use of internal methods to use this context.
  • Starts removing uses of .bind

@dmsnell
Copy link
Contributor

dmsnell commented Feb 19, 2018

is there any way to do this in pieces or is the plan to make the changes all at once?

@beaucollins
Copy link
Contributor Author

We can do this in pieces, I thought I would publish this since we started tackling the same stuff. I was trying to see how flowtype would fit into the project an started fixing all the things it started giving me errors about.

@beaucollins beaucollins changed the base branch from master to cleanup/auth February 19, 2018 19:39
* @param {String} id - the bucket object id to fetch
* @param {bucketStoreGetCallback} - callback once the object is fetched
*/
get( id: string, callback: ( ?Error, ?BucketObject ) => void ): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

why not standardize on a Promise interface? that would let people more easily use async/await too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats the goal, this is just keeping backwards compatibility right now.

export type BucketObjectRevision = {
id: string,
version: number,
data: {}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

sidenote: I find this extremely helpful. the hardest part in reading through this code or in the Simperium server's code isn't know when a type is invalid, but knowing when a collection of properties is supposed to be a given type, or knowing what properties to supply when a hidden remote function expects certain ones.

Copy link
Contributor Author

@beaucollins beaucollins Feb 19, 2018

Choose a reason for hiding this comment

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

I hope with fully defined jsondiff types this will shine a blazing light on all expected properties in the diffing/syncing magic.

var operation = {
type Modify = 'M';
type Remove = '-';
type OperationType = Modify | Remove;
Copy link
Contributor

Choose a reason for hiding this comment

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

for funnies check out the utility types

const operations = {
  Modify: 'M',
  Remove: '-'
};

type Operation = $Values<typeof operations>;

something like that

@roundhill roundhill changed the base branch from cleanup/auth to master February 26, 2018 22:37
@roundhill
Copy link
Contributor

The linter is complaining about a few things:

node-simperium/src/simperium/bucket.js
   61:2   warning  Mixed spaces and tabs                    no-mixed-spaces-and-tabs
   84:30  error    'Bucket' was used before it was defined  no-use-before-define
  157:15  warning  There must be a space inside this paren  space-in-parens

Looks like some class definitions need to be moved around and some spacing cleanup.

@roundhill
Copy link
Contributor

I switched this to merge with master since I merged #56 already.

@beaucollins
Copy link
Contributor Author

For this:

   84:30  error    'Bucket' was used before it was defined  no-use-before-define

When it's class definitions it shouldn't be a problem so we could eslint-disable-next-line

@roundhill
Copy link
Contributor

When it's class definitions it shouldn't be a problem so we could eslint-disable-next-line

WFM, but we'll need to add that 7 times in channel.js

@beaucollins
Copy link
Contributor Author

I had considered moving type definitions for the main components into it's own file which fixes this problem.

I think the goal is to break this PR into smaller pieces too.

@beaucollins
Copy link
Contributor Author

#62 is the beginning of a smaller portion of this PR.

@beaucollins beaucollins closed this Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants