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.

Sign up for GitHub

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

Jump to bottom

Remove duplicated WebEncoders class #93

Closed
dougbu opened this issue Feb 18, 2016 · 7 comments
Closed

Remove duplicated WebEncoders class #93

dougbu opened this issue Feb 18, 2016 · 7 comments
Assignees
@dougbu
Milestone
1.1.0-preview1

Comments

@dougbu
Copy link
Member

dougbu commented Feb 18, 2016

A big chunk of Microsoft.AspNetCore.WebUtilities.WebEncoders (for base64url-encoding and -decoding) is duplicated in Microsoft.AspNetCore.DataProtection.WebEncoders. With aspnet/HttpAbstractions#563, these classes are getting less aligned and DataProtection can't take advantage of the expanded API.

After a few offline discussions, have two ways to remove this wart:

  1. Move WebEncoders from HttpAbstractions to Common
    • #if the code such that it is placed in Microsoft.AspNetCore.DataProtection.Internal or Microsoft.AspNetCore.WebUtilities, depending on a flag.
      • In the DataProtection version, could make the class internal and leave it in Microsoft.AspNetCore.DataProtection if anyone feels strongly.
    • Use the new Common version in both DataProtection and HttpAbstractions (where it remains publicly exposed).
    • Leave Antiforgery and MVC alone; they will continue to rely on the public class in HttpAbstractions.
  2. Reuse the approach we took for ActivatorUtilities and place WebEncoders as an internal class in Microsoft.Extensions.Internal then expose it through a facade in HttpAbstractions. That avoids the oddity of a public class in a Blah.Blah.Blah.Sources package and makes the moved class more consistent w/ others in Common. But it involves more ceremony.
@Eilon Eilon added this to the 1.0.0-rc2 milestone Feb 19, 2016
@Eilon
Copy link
Member

Eilon commented Feb 19, 2016

I believe we discussed doing option (1), right?

@dougbu
Copy link
Member Author

dougbu commented Feb 19, 2016

believe we discussed doing option (1), right?

Yup, Then I went back and saw going that way was unlike the other .Sources code in Common. So I wanted to triple-check and see if anyone else wanted to weigh in.

@Eilon
Copy link
Member

Eilon commented Feb 19, 2016

I think it's the only option that makes sense, so I don't think there's any other reasonable way to go, no? We don't want a façade, and we don't want a public class for this...

@Eilon Eilon modified the milestones: 1.0.0, 1.0.0-rc2 Apr 2, 2016
@dougbu
Copy link
Member Author

dougbu commented May 25, 2016

Suggest this is cleanup we can postpone 'til 1.0.1. We aren't planning to make a change in what's publicly exposed here.

@dougbu
Copy link
Member Author

dougbu commented May 26, 2016

/cc @Eilon @danroth27

@Eilon Eilon modified the milestones: 1.0.1, 1.0.0 May 26, 2016
@Eilon
Copy link
Member

Eilon commented May 26, 2016

Moved.

dougbu added a commit that referenced this issue Jul 12, 2016
@dougbu
To remove, we must first add
9a241a0 
- part of #93
- copy `WebEncoders` into Common repo
- add hacks to work around dotnet/cli#3831 and NuGet/Home#3118
dougbu added a commit to aspnet/DataProtection that referenced this issue Jul 12, 2016
@dougbu
To de-dupe, we must remove
4f30ddd 
- part of dotnet/extensions#93
- use `WebEncoders` from Common repo

Also let VS have its way w/ test `.xproj` files
dougbu added a commit to aspnet/HttpAbstractions that referenced this issue Jul 12, 2016
@dougbu
To de-dupe, we must remove (again)
9a28932 
- part of dotnet/extensions#93
- use `WebEncoders` from Common repo
- leave some of `WebEncodersTests` to ensure resources are brought in correctly
This was referenced Jul 12, 2016
Closed
Merged
Merged
dougbu added a commit that referenced this issue Jul 13, 2016
@dougbu
To remove, we must first add
fe2bf2c 
- part of #93
- copy `WebEncoders` into Common repo
- add hacks to work around dotnet/cli#3831 and NuGet/Home#3118
@dougbu
Copy link
Member Author

dougbu commented Jul 13, 2016

@dougbu dougbu closed this as completed Jul 13, 2016
@ycrumeyrolle ycrumeyrolle mentioned this issue Jun 7, 2017
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@dougbu dougbu

Labels
None yet
Projects
None yet
Milestone
1.1.0-preview1
Development

No branches or pull requests
2 participants
@Eilon @dougbu