Skip to content

[draft] Feat/serializer context decoupling #512

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

Conversation

wisepotato
Copy link
Contributor

@wisepotato wisepotato commented May 23, 2019

The main goal of this PR is to counteract the coupling observed in JADNC. Let's make it more testable!

  • No more JsonApiContext dependency in Controllers
  • The entity is set via ResourceGraph in the middleware now
  • no more ApplyContext<T> in the controller, making for a coupling that is not needed
  • get all tests working

BUG FIX

  • reproduce issue in tests
  • fix issue
  • bump package version

FEATURE

  • write tests that address the requirements outlined in the issue
  • fulfill the feature requirements
  • bump package version

@wisepotato wisepotato added this to the v4.0 milestone May 23, 2019
@wisepotato wisepotato requested a review from maurei May 23, 2019 11:26
@wisepotato
Copy link
Contributor Author

This would also provide a general fix for #251

@wisepotato
Copy link
Contributor Author

A new controller would like ArticlesController, so no more JsonApiContext.

@milosloub What are your thoughts on this? I would say it makes for a more clean and conscise controller with the resources that it needs, and nothing more, allowing for better testability

        public ArticlesController(
            IJsonApiOptions jsonApiOptions,
            IResourceGraph resourceGraph,
            IResourceService<Article> resourceService) 
            : base(jsonApiOptions, resourceGraph, resourceService)
        { }

@milosloub
Copy link
Contributor

@wisepotato I have to look more into this. This is really big stuff, I have to spend more time to this. Thanks

@wisepotato
Copy link
Contributor Author

Discussed backward compatibility with @maurei , this will be an afterthought, after fully making what I want I'll try it in one of our V3 projects and see what breaks, and accomodate for that. I'm guessing that's something you would like to see @milosloub 💃

@maurei maurei changed the title Feat/serializer context decoupling [draft] Feat/serializer context decoupling Jun 25, 2019
@wisepotato wisepotato changed the base branch from feat/serializer-context-decoupling to master July 2, 2019 10:16
@wisepotato wisepotato requested review from maurei and removed request for maurei July 2, 2019 13:08
@wisepotato
Copy link
Contributor Author

wisepotato commented Oct 9, 2019

Being done in #558

@wisepotato wisepotato closed this Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants