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

Multi-target to reduce dependencies #3033

Open
mus65 opened this issue Oct 27, 2024 · 9 comments · May be fixed by #3080
Open

Multi-target to reduce dependencies #3033

mus65 opened this issue Oct 27, 2024 · 9 comments · May be fixed by #3080

Comments

@mus65
Copy link

mus65 commented Oct 27, 2024

Currently, Microsoft.Playwright has a few dependencies which are all unnecessary on modern .NET because they are part of the framework itself. This will become even more of an issue with .NET 9 since NuGet Audit will start complaining about CVEs in transitive dependencies, so all consuming modern. NET applications will get false positive warnings about e.g. a CVE in System.Text.Json .

Multi-targeting would allow to remove these dependencies on modern .NET . It would also allow to conditionally make use of newer APIs in modern .NET.

I could make a PR for this and already looked into this. Unfortunately this is not as simple as editing the csproj since quite a few new warnings are introduced in the net8.0 target which would need fixing or suppressing. So I first wanted to get some feedback on whether a PR for this would be accepted.

@mxschmitt
Copy link
Member

Which CVEs are you referring to? We aim to fix security vulnerabilities so we did it with #3016.

@mus65
Copy link
Author

mus65 commented Oct 29, 2024

I'm referring to CVEs in general. Currently a CVE in System.Text.Json would affect both .NET Framework and modern .NET applications. With multi-targeting, only .NET Framework applications would be affected since the dependency isn't even needed on modern .NET. So something like #3016 would not be necessary in the first place (at least for modern .NET users).

@mxschmitt
Copy link
Member

Lets collect feedback for it. So far we didn't see much demand for it. Multi-targeting means having compiler directives inside the code which adds a lot of overhead for us to maintain the library, also testing wise these are then different code-paths etc.

@kblok
Copy link
Contributor

kblok commented Nov 5, 2024

My two cents. Some libraries have different implementations, whether you use the netstandard version or the .NET 8 version, for good or bad.
The good part is that you might get a bump in performance if you use .NET-specific libraries and fewer dependencies. The bad part is that you might have different behaviors (potential bugs) on different platforms.

@RenderMichael
Copy link
Contributor

This would be very helpful. It would prevent modern apps from breaking on any future CVEs (it recently killed my build process)

Compiler directives should not be necessary for the case of System.Text.Json, Playwright just needs to target netstandard2.0 and net8.0 and only import STJ on .NET standard. .NET 8+ projects using Playwright will use STJ automatically, without any extra work from Playwright.

@RenderMichael
Copy link
Contributor

The bad part is that you might have different behaviors (potential bugs) on different platforms.

This is the case no matter what when you're targeting .NET standard unfortunately. It also doesn't apply for STJ, since you can align the version of the nuget package with .NET 8.

The only conceivable downside to this is somewhat longer build times.

@RenderMichael
Copy link
Contributor

Nuget's recent change to flag transitive dependencies will probably make this a more and more pressing issue for people. The three public packages that core Playwright ships with (System.Text.Json, Microsoft.Bcl.AsyncInterfaces, and System.ComponentModel.Annotations) are all polyfills which come built-in with modern .NET:

<PackageReference Include="System.ComponentModel.Annotations" Version="5.0.0" />
<PackageReference Include="System.Text.Json" Version="6.0.10" />
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="8.0.0" PrivateAssets="All" />
<PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="6.0.0" />

All of these could be removed, and PW could ship with no public dependencies

mus65 added a commit to mus65/playwright-dotnet that referenced this issue Dec 8, 2024
- add net8.0 target to projects
- fix all compiler warnings introduced by the new targets
- removed unneeded dependencies since they are part of the
  .NET runtime

fixes microsoft#3033
@mus65 mus65 linked a pull request Dec 8, 2024 that will close this issue
mus65 added a commit to mus65/playwright-dotnet that referenced this issue Dec 8, 2024
- add net8.0 target to projects
- fix all compiler warnings introduced by the new targets
- removed unneeded dependencies since they are part of the
  .NET runtime

fixes microsoft#3033
@mus65
Copy link
Author

mus65 commented Dec 8, 2024

Opened PR #3080 . I know there was no clear decision to do this, but I hope the PR makes it easier to decide whether this is worth doing.

@thomhurst
Copy link

Multi-targeting means having compiler directives inside the code which adds a lot of overhead for us to maintain the library, also testing wise these are then different code-paths etc

Not necessarily? If the packages bring in the same namespaces + signatures as what the later frameworks have, then you don't need any if statements. So there codepaths stay the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants