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

Convert JWT library to dotnetcore #60

Closed
wants to merge 7 commits into from

Conversation

alistair
Copy link

@alistair alistair commented Sep 29, 2016

I believe this is now finished but would like a good review of the changes I have made. I don't have VS and have instead been using vim/project rider on linux to work on this so please test on windows with VS.

This pull request converts the project to dotnetcore. It produces a nuget package targeting .NET 3.5 and NETSTANDARD1_3 ( which corresponds to .NET 4.5.1 i believe )

Unit tests are only run against the NETSTANDARD1_3 version of code as I was unable to get the other version to run at all. This seems like an outstanding bug with dotnet.

@abatishchev
Copy link
Member

Great! Thanks for your work and thanks for sharing it! I'm absolutely support switching from any other Json implementation to Json.Net.

Please also see #55. I logged this idea there.

@alistair alistair changed the title [WIP] Convert JWT library to dotnetcore Convert JWT library to dotnetcore Oct 2, 2016
@alistair
Copy link
Author

alistair commented Oct 2, 2016

@abatishchev I consider this ready now. Do you?

Copy link
Member

@abatishchev abatishchev left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot for this work! I left few comments/questions.

EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "JWT.Tests", "tests\JWT.Tests\JWT.Tests.csproj", "{BF568781-D576-4545-A552-4DC839B1AF14}"
Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "JWT", "src\JWT\JWT.xproj", "{00000000-0000-0000-0000-000000000000}"
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why it's empty guid now?

Copy link
Author

Choose a reason for hiding this comment

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

I will have to double check. I updated the solution using project rider so im not 100% confident it is correct. I don't actually have VS.

Copy link
Member

Choose a reason for hiding this comment

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

I'll try to compile in VS2015 once get access to my dev vm

@@ -15,7 +15,11 @@ public static class JsonWebToken
/// <summary>
/// Pluggable JSON Serializer
/// </summary>
#if !NETSTANDARD1_3
Copy link
Member

Choose a reason for hiding this comment

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

I think we should jump 2.0 and make this change for either platforms

Copy link
Author

Choose a reason for hiding this comment

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

Im assuming you mean BOTH platforms. I will make this change then.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, right


using FluentAssertions;

using Microsoft.VisualStudio.TestTools.UnitTesting;
using Xunit;
Copy link
Member

Choose a reason for hiding this comment

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

So you're switching over Xunit?

Copy link
Author

Choose a reason for hiding this comment

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

yes. xunit is like the default dotnetcore unit testing framework. All the examples Ive seen use it and I wouldn't have a clue how to setup anything else :)

Copy link
Member

Choose a reason for hiding this comment

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

Love this! Just wanted to make sure, was going to switch over Xunit anyway with v2


namespace JWT.Tests
{
[TestClass]
public class EncodeTests
{
private static readonly Customer _customer = new Customer { FirstName = "Bob", Age = 37 };

private const string _token = "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJGaXJzdE5hbWUiOiJCb2IiLCJBZ2UiOjM3fQ.cr0xw8c_HKzhFBMQrseSPGoJ0NPlRp_3BKzP96jwBdY";
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't then we use these variables as test's parameters in conjunction with [Theory]?

Copy link
Author

Choose a reason for hiding this comment

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

Probably, I was mostly concerned with making the most minimal of changes required to get this working.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, makes sense, this would be intend too. Let's keep it as is, not a big deal, not a big difference either.

"JWT": "1.3.7-*"
},
"frameworks": {
/*"net451": {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be 3.5 but why commented?

Copy link
Author

Choose a reason for hiding this comment

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

xunit only supports >= net451. I commented this because I was getting errors when attempting to run the tests. I will try track down the dotnet issue I was reading about this which might give some more context.

Copy link
Member

Choose a reason for hiding this comment

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

This might be another reason to bump the version to 2.0 since is a breaking change. Personally I don't mind at all.

@abatishchev
Copy link
Member

hey @alistair, i'm sorry it ran out of my scope. how do you think is everything ready to be merged?

@abatishchev
Copy link
Member

Hi, I made all changes I was going to bring in v2 in this PR: #67. If you could take a look, maybe apply your changes on top of it, so we could bring the .NET Core compatibility as part of v2?

@abatishchev
Copy link
Member

abatishchev commented Mar 7, 2017

I merged v2 into master. Anybody, please update the pr/rebase it.

@nbarbettini
Copy link
Contributor

Since this PR was using the old (project.json-based) tooling, I can build a new PR for .NET Core support. Let me know if that works for you @abatishchev!

@abatishchev
Copy link
Member

This would be perfect, thanks!

@paddyfink
Copy link

Hey guys, do you have any release date for this feature please?

@abatishchev
Copy link
Member

Let's move the discussion to #72

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.

4 participants