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 Cancellation Token Support #28

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

VictorioBerra
Copy link
Contributor

This is my first pass at solving #25

@AlexanderKrutov

Major changes:

  • Remove net40 support from DataTables.Queryable
  • Update Sample to Core 2.1 and add EF Core with SQLite support

Can you look at this pretty closely and make sure that it looks okay? 98a4a98 I had to try and tap into the internals of EFCore to get access to the ToListAsync and CountAsync goodies.

Also, it would super nice if we had some really basic tests and TravisCI. We could have some broad integration tests just as smoke tests. I hit all the Samples and they all work perfectly. I did not make any major changes to the actual views or DT libraries but i made a small change for the POST example to POST JSON instead of form post.

Victorio Berra added 6 commits October 29, 2018 12:38
… initalization of the instance is now asynchronous and can take a CancellationToken.
… Core and SQLIte.

I used the same views here and basically the same layout. I tried to keep all code exactly the same and just move to a core host using EF Core and SQLite.
@VictorioBerra
Copy link
Contributor Author

VictorioBerra commented Oct 29, 2018

See this issue on why Task.Factory.StartNew() is not good in aspnet core/ef core.

dncuug/X.PagedList#61

The async implementation is using Task.Factory.StartNew() but that is not good in ASP.NET environment, it is better to be using the approach done by Entity Framework here because that is not creating a new thread.

@VictorioBerra
Copy link
Contributor Author

VictorioBerra commented Oct 29, 2018

Maybe just doing return await Task.Run(() => superset.ToList(), cancellationToken); is good enough instead of offloading all that to EF by detecting the query type and all that? Then you could keep your net40 support.

I am not sure if there is a big difference, I would need to check the ExecuteAsync code of EF Core.

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.

None yet

1 participant