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

Add support for .net core #162

Open
2 of 6 tasks
joukevandermaas opened this issue Jun 12, 2017 · 14 comments
Open
2 of 6 tasks

Add support for .net core #162

joukevandermaas opened this issue Jun 12, 2017 · 14 comments

Comments

@joukevandermaas
Copy link
Owner

joukevandermaas commented Jun 12, 2017

This is an exploratory issue. I'm assuming there will be quite some work here, so this can serve as something of a todo list.

  • Change the project structure to support multiple targets (done by @laurence79)
  • Convert the 'Common' project to netstandard 2.0 (Upload a file and return json api #171)
  • Rebase on master (port new features implemented since the start of this work)
  • Re-enable stylecop/fxcop
  • Move as much as possible into the 'Common' project (perhaps wrapping some target-specific things in wrappers)
  • Clean up the tests (currently there is a lot of duplication, and some tests are in the wrong project IMO)
@laurence79
Copy link
Contributor

I've done some exploratory work on this which you can see at laurence79/saule/feature/net-core

I sideways copied all the classes and tests from the existing projects to new netcore projects.
The approach I have taken is to end up with 2 separate nuget packages - a classic ASP.net one and an aspnetcore one. I can't see how else we can support both frameworks given their entirely different dependencies - happy to be corrected here though.

Major changes that were required:

  1. Convert PreprocessingDelegatingHandler to an IActionFilter.
  2. Convert JsonApiMediaTypeFormatter to separate input and output formatters.
  3. Convert usage of actionContext.Request.Properties to actionContext.HttpContext.Items.
  4. Make alternative arrangements for surfacing errors - HttpError no longer available
  • Input formatters can't influence responses, only modelstate.
  1. Change the use of HttpConfig to MvcOptions.

All of the old tests have survived with only slight changes, and they all pass.

It would be sensible I think to extract out common functionality from the old and new libraries to a shared common library, probably using netstandard 2.0, although this will need the old library to target .NET Framework 4.6.1.

Let me know your thoughts 😊

@laurence79
Copy link
Contributor

Ok, further reading shows that we could indeed target both netcore and net4xx in the same package.

http://blogs.microsoft.co.il/iblogger/2017/04/05/easily-supporting-multiple-target-frameworks-tfms-with-vs2017-and-nuget/

Would you prefer this approach?

@laurence79
Copy link
Contributor

I've decided I also prefer that approach 😊

Latest code includes the following projects (tests not listed)

Saule.Common - Shared Functionality
(targets net452 & netcoreapp2.0)

Saule.AspNet - ASP.Net Specific classes
(targets net452)
(references Saule.Common, System.Web.dll etc.)

Saule.AspNetCore - ASP.Net Core Specific Classes
(targets netcoreapp2.0)
(references Saule.Common, Microsoft.AspNetCore.Mvc etc.)

Saule - For Nuget Packaging
(targets net452 & netcoreapp2.0)
(references Saule.AspNet or Saule.AspNetCore depending on target condition)

@joukevandermaas
Copy link
Owner Author

@laurence79 awesome work! I think it makes sense for Saule.Common to target .net standard (probably 1.6?). One thing that worried me was the existing people targeting the Nuget package Saule, but it seems that you have figured out a solution there as well.

I'm guessing some of the actual logic that's currently in the formatter and delegating handler must move into Saule.Common somehow. I think it's important from a contributions point of view that the two ASP specific projects are as small as possible. Some of the integration tests will necessarily need to be duplicated, but that's unavoidable. That said, I think there are some integration tests that could be unit tests.

How do you think we should proceed with getting this new stuff merged into the main Saule repo? Are there any blockers? I would really love to enable you, me and potential others to collaborate on this effort.

@laurence79
Copy link
Contributor

Thanks, I agree.

Perhaps if you start a new branch, say feature/net-core similar to how I have done in my fork. I'll then do a PR into that branch, then you, I, and others can fork that branch and submit PRs?

@joukevandermaas
Copy link
Owner Author

OK, I rebased your branch on master and pushed that in this repo. I also added you as collaborator so you can work in this repo and potentially help reviewing PRs into the feature/net-core branch.

I hope we can get this merged into master pretty smoothly once everything seems ready. I'll try to create a more up-to-date checklist in this issue soon so we know what the status is.

@joukevandermaas
Copy link
Owner Author

I've spent a bit of time on this in #177.

Basically my thoughts are currently like this:

  • I want to move common to netstandard 2, which means bumping the framework version to 4.6.2 and only supporting .Net Core 2.0
  • Since that's a breaking change and we'll need to bump the major version, I also want to remove the 'old' deprecated way of doing setup
  • I also want to slightly clean up the pagination API since it's super confusing

I'll continue in #177 to make the tests and the appveyor build pass.

@prmces
Copy link

prmces commented Jan 12, 2018

Hello Guys!!!

Great library.... perfect..

It is already possible to use the new features for the .net core??

If not there is any release date??

Thanks in advance ..

Paulo

@joukevandermaas
Copy link
Owner Author

Hi Paulo,

I kind of hit a dead end with the migration @laurence79 started in a separate branch. I think we need to prepare the master branch so something similar can be merged into the main releases. Because of my schedule right now I cannot really maintain two separate branches and keep feature parity.

I think the work @laurence79 did informed the direction of this project in a few important ways that open up more contributions:

  1. We need to extract all 'real' logic into Saule-specific classes. In other words, no web api related abstractions should directly do anything, instead they should call into an abstraction. Most of these abstractions are already worked out to some extend in the feature/net-core branch.
  2. We need to extract the web api portion of Saule into a separate project. This effectively means we'll have Saule-Core and Saule-WebApi. Saule-Core will contain all the logic and abstractions, Saule-WebApi will bind these to web api concepts.

All of the above can and should happen in master. After that work is done (or meanwhile, really), we can create .Net core implementations for the same abstractions already in Saule-Core. I cannot commit to a timeline for this as it will largely depend on outside contributions.

All that said, it is definitely possible to use the stuff in feature/net-core. It will take some effort on your part, and it will not have feature parity, but it mostly all works.

@SuricateCan
Copy link

Hi @joukevandermaas,
This library is the most complete and easy to use implementation for jsonapi I could find, however, I am working with .NET core and it not being fully supported yet is a shame.
I'd like to contribute, at least for feature parity.
Do you have any recomendations as to what to do and to not?

@joukevandermaas
Copy link
Owner Author

@SuricateCan i would love to have support for core but due to my schedule i just can't put in the time. After the experiment documented above i think the best approach is as follows:

  1. Create a net standard project with dependency on neither asp web api nor core.
  2. Create two projects which implement the public interface for core and framework.
  3. Move as much as possible of the current code into the net standard project. If code is currently implemented as part of a framework api, we can move the implementation out into an interface and implement it for both targets.

This is basically the approach mentioned above. But i think we should do step 1 in master so new contributions work for both targets. Doing this in parallel with new prs seems impossible to manage.

So if you want to take a stab at that in a pr I'm more than happy to review and merge it.

@SuricateCan
Copy link

Great, I'll work on small compatible PRs so it's easier to merge,

@guillemsola
Copy link

I have colleagues at work who are using Saule as their json API implementation of choice. As per this thread I understand net core is still not an option. Muy question is, is that because this other implementation works for that matter https://github.com/json-api-dotnet/JsonApiDotNetCore

@joukevandermaas
Copy link
Owner Author

@guillemsola The reason is, I don't use dotnet core at work and I don't have time to do this outside of work. It is a shitty situation but that's what it is.

However, the fact that there are other implementations that are compatible is good. When I first created Saule, there were no implementations for web api.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants