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

chore(deps): update dependency system.text.json to v8 - autoclosed #237

Closed
wants to merge 1 commit into from
Closed

chore(deps): update dependency system.text.json to v8 - autoclosed #237

wants to merge 1 commit into from

Conversation

renovate[bot]
Copy link
Contributor

@renovate renovate bot commented Jul 12, 2024 •
edited
Loading

Mend Renovate

This PR contains the following updates:

Package Change Age Adoption Passing Confidence
System.Text.Json (source) 6.0.9 -> 8.0.4 age adoption passing confidence

Release Notes

dotnet/runtime (System.Text.Json)

v8.0.4: .NET 8.0.4

Release

v8.0.3: .NET 8.0.3

Release

v8.0.2: .NET 8.0.2

Release

v8.0.1: .NET 8.0.1

Release

v8.0.0: .NET 8.0.0

Release

What's Changed

Full Changelog: dotnet/runtime@v8.0.0-rc.2.23479.6...v8.0.0

v7.0.4: .NET 7.0.4

Release

v7.0.3: .NET 7.0.3

Release

v7.0.2: .NET 7.0.2

Release

v7.0.1: .NET 7.0.1

Release

v7.0.0: .NET 7.0.0

Release

Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

♻ Rebasing: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 Ignore: Close this PR and you won't be reminded about this update again.

  • If you want to rebase/retry this PR, check this box

This PR was generated by Mend Renovate. View the repository job log.

@renovate renovate bot force-pushed the renovate/major-dotnet-monorepo branch 2 times, most recently from 9b68e34 to 14ff608 Compare July 12, 2024 12:41
@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jul 12, 2024 •
edited
Loading

@clrudolphi @gasparnagy we try to keep our dependencies up to date automatically. If a major versions should not be upgraded to, it is usually best to write a test that will fail when the version if upgraded. Otherwise an unwanted upgrade might slip through in the future.

@clrudolphi
Copy link
Contributor

IIRC we would prefer to target that older version in order to avoid version conflicts with Reqnroll.
I'm not presently able to work on code (away on holiday).
@gasparnagy please confirm whether we need that specific older version. If so, could you add the test that would fail and prevent the automated updater from letting that newer version from creeping in?

@gasparnagy
Copy link
Member

@mpkorstanje I understand the motivation, but this is not that easy. (Or at least I feel so.) Let me explain the situation and you can give a suggestion about how this should be handled.

The cucumber messages currently contains two assets:

  • the message classes (that Reqnroll would like to use in general, for example for storing Gherkin AST)
  • the message JSON serialization helper (that is essentially just a configuration of the System.Text.Json library) - this is going to be used only in special cases when someone will activate the cucumber messages logging

In .NET testing libraries (like Reqnroll) the dependencies are common problems, because typically users would like to test their own code with it that has it's own dependencies. As JSON serialization is a common concern, this has caused a lot of issues in the past: if the tool (Reqnroll, SpecFlow) specifies the dependency (e.g. System.Text.Json 8.0.4), the tool cannot be used to test the SUT that uses a lower dependency version (or it will implicitly upgrade the version used by the SUT, so at the end it will not test the product with its own dependencies, causing potential false test results). We had such issues in the past.

So far we have used two strategies to avoid this problem:

  1. Try to avoid using dependencies that are potentially used by the SUT. E.g. we had our own JSON serializer for SpecFlow. (But I would like to get rid of that, because it is quite an overhead.)
  2. Set the dependencies to the minimum required version. With NuGet packages it means: any System.Text.Json with version greater than 6.0.0 can be used. So it is not "we" that select the version, but the SUT. We just declare what we are compatible with. (SpecFlow/Reqnroll use this strategy quite extensively for test runner (NUnit/MsTest/xUnit) and other integrations.)

The package System.Text.Json is even more special, because nowadays it is part of the framework anyway, so you don't need to explicitly choose a version - it will use the one comes with your .NET version (e.g. with .NET 8). There are a few still commonly used .NET version (the .NET Framework v4.7, v4.8), that does not have this yet. These frameworks are in the compatibility target .NET Standard 2.0, that we (and most libraries) target. This is also visible in the version "usage" of the package, the v6.0.0 is used 3 times more than v8.

So in short: actually the System.Text.Json difference is only relevant for .NET Framework v4.7, v4.8, for any other, the framework built-in one will be used (if that is newer).

As I said, I don't really know what the best would be, but I see the following options, that would be suitable for Reqnroll (that is currently the only user of the messages lib, I think).

  1. Keep the version at v6.0.0 and make a comment that we on-purpose delayed upgrading
  2. Make a release with v6.0.0 (that Reqnroll can use) and only after upgrade to 8.0.0 (and hope that there will be no changes until the System.Text.Json versioning is better settled)
  3. Separate the message classes and the JSON serialization to two separate packages (and Reqnroll will only use the classes and copy-paste the JSON serialization config as code)
  4. We use a custom JSON serializer for the messages and not one from a dependency (which one? how?)
  5. Reqnroll forks the project and makes its own package with its own required dependencies (that is quite a big work, so I would like to avoid this).

What do you think?

@clrudolphi
Copy link
Contributor

clrudolphi commented Jul 16, 2024 via email

@mpkorstanje
Copy link
Contributor

Then you effectively have the same problems as the Java implementation. So I'd got for a slight variation of your suggestions.

  • Remove the dependency on System.Text.Json.
  • Remove any code that uses System.Text.Json.
  • Requires the MessageToNdjsonWriter to be constructed with a function that converts a message to json and provide that function when and if it is needed. E.g:

messages/java/src/main/java/io/cucumber/messages/MessageToNdjsonWriter.java

Lines 44 to 48 in fdbb9fb

public interface Serializer {
void writeValue(Writer writer, Envelope value) throws IOException;
}

edit: Got sniped. What @clrudolphi said. 😄

@renovate renovate bot force-pushed the renovate/major-dotnet-monorepo branch 5 times, most recently from 39ebcba to dc3e80e Compare July 23, 2024 22:03
@renovate renovate bot force-pushed the renovate/major-dotnet-monorepo branch 2 times, most recently from cf6e991 to 8233dcb Compare July 29, 2024 19:46
@clrudolphi
Copy link
Contributor

Submitted PR #241 to address this. Moved the System.Text.Json dependency in to the testing library. The deployable Nuget package can be built without that dependency.

@renovate renovate bot force-pushed the renovate/major-dotnet-monorepo branch from 8233dcb to bc67620 Compare August 2, 2024 12:47
@renovate renovate bot changed the title chore(deps): update dependency system.text.json to v8 chore(deps): update dependency system.text.json to v8 - autoclosed Aug 2, 2024
@renovate renovate bot closed this Aug 2, 2024
@renovate renovate bot deleted the renovate/major-dotnet-monorepo branch August 2, 2024 19:06
@mpkorstanje mpkorstanje mentioned this pull request Aug 15, 2024
Merged
7 tasks
@Romfos Romfos mentioned this pull request Aug 27, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gasparnagy gasparnagy Awaiting requested review from gasparnagy

Assignees
No one assigned
Labels
🤖 dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mpkorstanje @clrudolphi @gasparnagy