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

Bump to .Net 8 + Update Nuget Packages to non vulnerable version #278

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

NikoMix
Copy link
Contributor

@NikoMix NikoMix commented Aug 30, 2024

Since I wanted to work some more with Statiq and figured out there had been some issues to integrate it with the current LTS (.net 8) version, started to work on an commit. Only shortly before wrapping up the PR figured out #270 had been open - but already since January, so assuming it's somewhat stale.

A few notes:

  • Statis.Tables had not been updated on EPPlus due to a more restrictive license change (pay to use)
  • A few breaking changes have been introduced, with the update of some nuget packages
    • Loading of SixLabors Image changed, where format is not an output parameter anymore, instead read through image metadata
    • Asserts have been restructured to use Assert.That() instead extension methods like .True()/.False()/.Equals()/etc.
    • SemVer is now SemanticVersioning
    • RecyclableMemoryStreamManager is instanciated through a options object, instead of parameters and property setters
    • XML Navigation with .Descendents().OfType() is obsolete and use of .Descendants() encouraged (uses the same under the hood)
    • Added few ArgumentNullChecks (where it would not really interrupt any code)
  • Quite a lot of new Code Analyzers have been introduced, but for now put as warning to keep the change as non-intrusive as possible (to avoid a super large change)

Builds on my machine, but not sure if/what/how to get the test's run, so a proper validation would still be pending - but compiles for me :) Tried to avoid adding any new or removing any existing code/features.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an actual mistake by me in translating the "old" asserts into the "new" - so corrected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically Struct is actually more correct, but seems that prior frameworks reported SpecificKind as Structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With more recent Handlebars version, the BlockHelper needs to be started by # - can be seen in the official handlebars repository. ⚠️ CAUTION: This is likely to be a breaking change to prior Statiq.Framework users - but would make sense to follow the "official" handlebars behaviour

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied Globalization on TestFixture, however only two tests had been affected on my local machine by the coma seperator due to german localization.

@@ -541,7 +550,7 @@ public async Task ConvertsSearchableField(object value, string expected)

// Then
TestDocument indexDocument = results.ShouldHaveSingleWithDestination(GenerateLunrIndex.DefaultScriptPath.ChangeExtension(".index.json"));
indexDocument.Content.ShouldContain(expected);
indexDocument.Content.ShouldContainWithoutWhitespace(expected); // ShouldContain is clipped to first 100 chars, but occurrence is beyond. This method is not clipped; Workaround
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a little more tricky - ShouldContain is (now?) clipped to 100 chars and the match is actually in the testdocument content - however all further text is cut-off with "..." as such the test fails. ShouldContainWithoutWhitespace is the only extension method, which is not clipping the actual. Tried to explain through comment, so it's not reversed as it's not very "intuitive"

@NikoMix
Copy link
Contributor Author

NikoMix commented Aug 31, 2024

Last issue center around Statiq.Sass, which does not seem to be compatible with ARM64 which seems to be the case for macos runners from GitHub Actions (see open issue here: xoofx/SharpScss#19) a workaround might fix the ability to run on the agent - but would not run on end-users working with Statiq on macos. @daveaglick would you have a take on that?

@daveaglick
Copy link
Member

Thanks for all the work on this! Not sure when I'll have a chance to review it, but I'm hoping to carve out time soon since it covers what was the last thing I wanted to do before finally releasing a 1.0 version.

@NikoMix
Copy link
Contributor Author

NikoMix commented Sep 5, 2024

Sure, happy to contribute and no pressure. Been looking to make the update for Statiq.Docs and .Web as well - but the Scss issue is making this particular pull request somewhat tricky as it seems either a dirty workaround (installing libscss for the agent on macos only) or replacing SharpScss (?). Both did not seem ideal, if you have any preference/alternative more than happy to take this over the finishing line and you can review at your leisure

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

Successfully merging this pull request may close these issues.

2 participants