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

S.R.InteropServices.JavaScript is part of the shared framework but doesn't have a ref assembly #39042

Closed
ViktorHofer opened this issue Jul 9, 2020 · 19 comments · Fixed by #40478
Assignees
Milestone

Comments

@ViktorHofer
Copy link
Member

System.Runtime.InteropServices.JavaScript is currently declared as part of the shared framework but doesn't have a reference assembly. As the project isn't OOB either, there is currently no way for a customer to reference the library besides directly referencing the implementation one from the shared framework directory.

Is the omission of a reference assembly intentional?

cc @kjpou1 @marek-safar @ericstj

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Infrastructure-libraries untriaged New issue has not been triaged by the area owner labels Jul 9, 2020
@ghost
Copy link

ghost commented Jul 9, 2020

Tagging subscribers to this area: @safern, @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@jkotas
Copy link
Member

jkotas commented Jul 9, 2020

This is internal library, for internal consumption only for now.

See #35573 (comment)

@safern
Copy link
Member

safern commented Jul 9, 2020

So then we should close the issue as by design?

@ericstj
Copy link
Member

ericstj commented Jul 9, 2020

Do we want to change the name to System.Private to indicate it is internal only? We've done so elsewhere (CoreLib, URI, XML).

@marek-safar
Copy link
Contributor

Do we want to change the name to System.Private to indicate it is internal only?

Is it worth it? We'll probably make the library public in the future (e.g. .NET7).

@stephentoub
Copy link
Member

Is it worth it? We'll probably make the library public in the future (e.g. .NET7).

I would think so. That's two releases away, including an LTS.

@ViktorHofer ViktorHofer added this to the 5.0.0 milestone Jul 12, 2020
@ViktorHofer ViktorHofer removed the untriaged New issue has not been triaged by the area owner label Jul 12, 2020
@ViktorHofer ViktorHofer self-assigned this Jul 30, 2020
@ericstj
Copy link
Member

ericstj commented Aug 4, 2020

Do we think its worth the risk to change the name at this point in the release? What could break if we change this name? cc @steveisok @akoeplinger

@lewing
Copy link
Member

lewing commented Aug 4, 2020

Nothing external should break at this point, the name is already different than the 3.2 counterpart. The name is referenced in a couple of places by the js internally, and at least one test helper.

@steveisok
Copy link
Member

I think this is pretty low risk and we should be able to change it.

@akoeplinger
Copy link
Member

akoeplinger commented Aug 4, 2020

I think I already did add the reference assembly as part of c652058.

@ViktorHofer
Copy link
Member Author

Thanks Alex. You missed removing the P2P in the test project (as it wasn't part of the targeting pack before):

<!-- Part of the shared framework but not exposed. -->
<ProjectReference Include="..\src\System.Runtime.InteropServices.JavaScript.csproj" />

@ViktorHofer
Copy link
Member Author

Submitted #40337 to clean this up. Meanwhile, this issue can be closed. Thanks all.

@steveisok
Copy link
Member

@ViktorHofer I still think we need to change the name since it's internal.

@steveisok steveisok reopened this Aug 4, 2020
@akoeplinger
Copy link
Member

I wasn't aware of this issue and that this is an internal library for 5.0 so I think we'll need to remove the reference assembly again.

@safern
Copy link
Member

safern commented Aug 4, 2020

I wasn't aware of this issue and that this is an internal library for 5.0 so I think we'll need to remove the reference assembly again.

We don't need to remove the ref assembly, we can make the ref to not be shipped if we want to:

https://github.com/dotnet/runtime/blob/master/src/libraries/NetCoreAppLibrary.props#L149

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 4, 2020

@safern Why would we keep the ref if we aren't shipping it as part of the targeting pack?

@safern
Copy link
Member

safern commented Aug 4, 2020

I don't know, because we plan on making it public further down the road and to simplify the change; I guess "if we want to" wasn't clear enough to express that 😄.

I guess I was just trying to express that we have libraries where the ref is OOB and the implementation Inbox. In this case the ref is not even OOB, so if we don't need the ref for anything, removing it sounds reasonable.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 4, 2020

@steveisok @akoeplinger can you please make a decision on what we should do here for 5.0? In master right now we expose a ref assembly which means the assembly isn't private atm. Should we remove the ref assembly and rename library or keep the ref and hence expose the lib?

@steveisok
Copy link
Member

I thought I said changing it would be low risk and something we should be able to do.

To be clear, if it's internal in 5.0, we should treat it as such and rename / remove the ref.

@lewing lewing added the arch-wasm WebAssembly architecture label Aug 5, 2020
steveisok pushed a commit to steveisok/runtime that referenced this issue Aug 6, 2020
steveisok added a commit that referenced this issue Aug 7, 2020
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this issue Aug 10, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.