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

Fable 5, not compatible with Fable 4 plugins #3962

Closed
MangelMaxime opened this issue Nov 24, 2024 · 4 comments · Fixed by #3963
Closed

Fable 5, not compatible with Fable 4 plugins #3962

MangelMaxime opened this issue Nov 24, 2024 · 4 comments · Fixed by #3963

Comments

@MangelMaxime
Copy link
Member

MangelMaxime commented Nov 24, 2024

Description

When releasing Fable 5.0.0-alpha.1, the tests didn't capture the fact that upgrading Fable version would make Fable 4 plugins not supported.

This happened because when running the tests, we don't upgrade Fable internal version so it was still checking against version 4.24.0

Solutions

  1. We ask Fable plugins to upgrade their FableMinimumVersion to 5.0.

    Pro we don't need to do anything on Fable side.

    Cons this will create a new shift in Fable ecosystem, because then Feliz will need to create a new major version. Which could cause a cascade of upgrade needed for others packages that depends on it.

    The problem is that package authors are not as active as before, so this cascade change can take some time.

  2. We adapt Fable logic to allows plugins requesting FableMinimumVersion = "4.0" to works with Fable 5.

    Pro Fable compiler plugins and others NuGet packages depending on them will need to do nothing. This seems also more aligned with the name of the property FableMinimumVersion. It says minimum version and not major version.

    Cons we will start to make custom logic allowing to support different span of Fable compiler plugins. This would allows us to say as long as Fable.AST stay stable we can support the same plugin for Fable 4, 5, 6, etc.

    This question also if we should not have used the version of Fable.AST assembly (I think .NET as version info in the assembly) instead of an arbitrary FableMinimumVersion property.

What do you think @ncave @dbrattli ?

@ncave
Copy link
Collaborator

ncave commented Nov 24, 2024

@MangelMaxime IMO staying with FableMinimumVersion = "4.0" makes sense, since Fable AST hasn't changed (so # 2 above).

@MangelMaxime
Copy link
Member Author

MangelMaxime commented Nov 24, 2024

I forgot to mention it, but I am also in favour of # 2.

@nojaf
Copy link
Member

nojaf commented Nov 25, 2024

5.0.0-alpha.1 is a different notation to how the 4.0.0-theta-018 was used in the past.
Regex over at

let private expectedVersionMatchesActual (expected: string) (actual: string) =
try
let r = System.Text.RegularExpressions.Regex(@"^(\d+)\.(\d+)(?:\.(\d+))?")
let parse v =
let m = r.Match(v)
int m.Groups[1].Value,
int m.Groups[2].Value,
if m.Groups[3].Success then
int m.Groups[3].Value
else
0
let actualMajor, actualMinor, actualPatch = parse actual
let expectedMajor, expectedMinor, expectedPatch = parse expected
// Fail also if actual major is bigger than expected major version
actualMajor = expectedMajor
&& (actualMinor > expectedMinor
|| (actualMinor = expectedMinor && actualPatch >= expectedPatch))
with _ ->
false
probably no longer matches.

@ncave
Copy link
Collaborator

ncave commented Nov 25, 2024

@nojaf

probably no longer matches.

No, that Regex just matches the version parts, should be ok, see #3963.

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.

3 participants