-
Notifications
You must be signed in to change notification settings - Fork 464
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
Conversation
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. |
This is the lowest version supporting cryptography
@abatishchev I consider this ready now. Do you? |
There was a problem hiding this 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}" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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]
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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": { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
hey @alistair, i'm sorry it ran out of my scope. how do you think is everything ready to be merged? |
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? |
I merged v2 into master. Anybody, please update the pr/rebase it. |
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! |
This would be perfect, thanks! |
Hey guys, do you have any release date for this feature please? |
Let's move the discussion to #72 |
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.