Library Refactor

[ShadowManu]

I'll just post so I can be more open about the plan I had with this branch, both with my internal team at ZOI and all contributors. Some things I have to say:

1. The core goal of this branch is to try to simplify the library logic, by trying to split feature logics in small functions instead of being merged with the same feature logics line-to-line in the same methods. As part of this is trying to give a more straightforward view on how to correctly implement the required recursions for collections, related resources, nested link and blah blah blah.
2. Last work I did was 2 days ago. I was not directly assigned as a task to solve this but I was interested in contributing to the project. At least I was able to do some work.
3. The actual status of the refactor has just reached the point of rethink->rewrite->correct-transpilation. I didn't even ran the tests so you can be pretty sure the thing is broken. However, the only lasting work is to run the tests and fix the mistakes I may've done in the proper places, taking special care on the TODO tags. If anybody feel brave macho enough to help me completing the refactor past the test (including the nesting tests), I think everything will be very appreciative. If not, well, I'll keep working on this from the 19th, if time and god allows it.

PD: I'll try to be available to discuss things about this pull request ;)

[ShadowManu]

Ha! Look, not even the test source compile for now ;). Comments still apply.

[jamesdixon]

Thanks for this! I'll do what I can to help out, but won't have much time for the next few weeks.

[ShadowManu]

Yep, some time available (and I was bored enough) to work on this and started checking out whats left on this. Most things are updated/cleaned up, and we are going on 28/37 tests passing (9 to go). Hopefully this gets done soon ;)

[ShadowManu]

2 tests to go, but I must stop now. There's a lot of different work I'll be focused on all this week.

[chamini2]

I'll take a look sometime this or the next week!

[jamesdixon]

Likewise! Thanks for your hard work on this.

[chamini2]

Some reminders:

• [x] remove outdated comment in data links and relationships links functions
• [x] finish new relation types API
• [x] refactor toJSON
• [x] remove index from import
• [ ] add flag to avoid removing attributes #56
• [ ] use fast package mentioned in #55

[chamini2]

@jamesdixon, regarding the library mentioned in #55, I don't think it works for our case because we just use Bookshelf's toJSON.

[chamini2]

@ShadowManu, @jamesdixon, this seems to be ready for a merge if the issue #56 should be addressed separately. If you agree that there should be an option to avoid the removal of attributes, I could do it in this branch.

[ShadowManu]

Issue #56 can be left out for later. I do believe we can do a major pre-release. We are not having breaking changes (except totally deprecating includeRelations), and adding some new features, but I do believe that just incrementing a minor version won't make it justice. Even more, I do believe the project has a stable concept and a stable implementation.

However, I do believe that battle-testing the library in the projects that we are using the library will provide us with feedback for bugs that somehow escaped the actual set of tests. (Especially our team that had to set quite some hacks around the mapper for unexpected/undesired behaviour.

For these reasons, do you guys agree on a proud 1.0.0-beta.0 prerelease? Better linting, docs, bugfixes, travis tests and the like can be done along the way.

PD: @chamini2 great work! I actually loved most of the refactors you did.

[chamini2]

Yeah, this feels to me like a 1.0.0-beta version for sure! The only other breaking change is the addition of the option enableLinks replacing the old disableLinks (which is not considered for deprecation, but could be).

Also agreed the issue #56 could be handled later.

[chamini2]

The query option says the following in the README:

query (object): an object containing the original query parameters. these will be appended to self and pagination links. Developer Note: this is not fully implemented yet, but following releases will fix that.

Is it fully implemented now?

[chamini2]

As seen in this example of the json api spec a links object insde of the data is not expected, yet we are currently adding one (and even testing for it!). Should we look into this or is it something with jsonapi-serializer?

[ShadowManu]

Resource Objects MAY contain a links property.

[jamesdixon]

@chamini2 @ShadowManu first off, I wanted to thank you both for the fantastic work that you've done on this refactor. My apologies for not being able to contribute as of late, but we're getting close to launching the beta of our product, which has kept my attention elsewhere.

All that said, I concur that a 1.0-beta is the way to go. Although this is a refactor, the library has been in use enough in our own projects, in addition to others, with very few issues, that it seems ready to hit 1.0.

With this refactor, I'm really excited to hopefully see some additional interfaces to other ORMs added to the project in the near future.

LGTM!