Skip to content

refactor(api): split and host api/index.js functions in *.api.js#367

Closed
machour wants to merge 2 commits intogitpoint:masterfrom
machour:api-banana-split-2
Closed

refactor(api): split and host api/index.js functions in *.api.js#367
machour wants to merge 2 commits intogitpoint:masterfrom
machour:api-banana-split-2

Conversation

@machour
Copy link
Copy Markdown
Member

@machour machour commented Sep 26, 2017

Following up on #361

Comment thread src/auth/auth.action.js Outdated
fetchUserEvents,
fetchStarCount,
} from 'api';
} from '../user/user.api';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have aliases, so that you can replace everywhere relative paths like ../user/user.api with user/user.api, should work as before.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I learned something new, thanks @lex111 !

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep, all thanks to babel-plugin-module-resolver

@machour
Copy link
Copy Markdown
Member Author

machour commented Sep 27, 2017

I'm having second thoughts about this refactoring, as these functions doesn't seem to belong there:

src/issue, src/user/, .. in GitPoint doesn't represent the v3 API endpoints, but represent instead the sections in the app.

It would make more sense to host the API related functions under src/api/v3/, and split them by endpoints. (api/v3/users, api/v3/repos, ..)

I also think we should consider renaming those functions in order to have more conventional names. fetchSearch for example doesn't feel right at all. v3.search.users() or v3.search.repos() seems a bit more standard as per GitHub naming.

I had a look at several GitHub clients apis over the last days, and I really love the naming node-github adopted. I've opened a proposal in #348 to switch to this library for v3, but I'm not sure how it would play with our redux/normalizr plans..
We can however adopt the same naming for our home made client.

Furthermore, we could even make use of this file in a little script to generate our API functions (only the one needed in the app). 🤔

Something to consider right after release Monday!

@housseindjirdeh
Copy link
Copy Markdown
Member

Thank you so much mate for taking the time to split these up in their separate domains.

I think this is tons better than having a single index file with all of our API logic.

That being said I kinda agree. Having everything named fetch isn't the nicest way of doing things and a convention would be nice. I also don't think it's a bad idea to have all the logic within api/v3 and not keep their concerns feature-based.

Will love to hear other's opinions on this as well. Also happy to merge this in now if you would like to deep-dive and refactor and fix things up next week or close this since we'll be doing that anyway :)

@machour
Copy link
Copy Markdown
Member Author

machour commented Sep 28, 2017

Closing this for now, we'll handle that more properly next week 🙌

@machour machour closed this Sep 28, 2017
@machour machour deleted the api-banana-split-2 branch September 30, 2017 23:15
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