Zaid-Ajaj / Femto

Femto is a CLI tool that automatically resolves npm packages used by Fable bindings
153 stars 13 forks source link

Automatic conflict resolution and dev dependency implementation #35

Closed Zaid-Ajaj closed 5 years ago

Zaid-Ajaj commented 5 years ago

Fixes #21 and Fixes #29

First of all: my head hurts 😅 everything seems to work like it should but I think some more manual tests would be greate (please @MangelMaxime 🙏)

Conflict Resolution

This PR implements conflict resolution when you require the same package multiple times for different version ranges, Femto will try to resolve a specific version that satisfies all version ranges. Example Fable.DateFunctions where it is a project reference of App.fsproj

  <PropertyGroup>
    <NpmDependencies>
      <NpmPackage Name="date-fns" Version=">= 1.0 lt 2.0" />
      <NpmPackage Name="date-fns" Version=">= 1.20 lt 2.0" />
    </NpmDependencies>
  </PropertyGroup>

Normally these different packages would be required by different projects but the sake of testing I am using a single project. Now running femto --preview against project App.fsproj will give the following:

[02:15:10 INF] Analyzing project C:\projects\Fable.DateFunctions\app\App.fsproj
[02:15:14 INF] Found package.json in C:\projects\Fable.DateFunctions
[02:15:14 INF] Using yarn for package management
[02:15:14 INF] Previewing required actions for package resolution
[02:15:15 INF] Fable.DateFunctions -> Install date-fns version 1.20.0 satisfies [>= 1.0 < 2.0 && >= 1.20 < 2.0]

Now adding an impossible version range such as >= 5.0 as follows:

  <PropertyGroup>
    <NpmDependencies>
      <NpmPackage Name="date-fns" Version=">= 1.0 lt 2.0" />
      <NpmPackage Name="date-fns" Version=">= 1.20 lt 2.0" />
      <NpmPackage Name="date-fns" Version=">= 5.0" />
    </NpmDependencies>
  </PropertyGroup>

Gives the following logs

[02:14:34 INF] Analyzing project C:\projects\Fable.DateFunctions\app\App.fsproj
[02:14:37 INF] Found package.json in C:\projects\Fable.DateFunctions
[02:14:37 INF] Using yarn for package management
[02:14:37 INF] Previewing required actions for package resolution
[02:14:39 ERR] Fable.DateFunctions -> Unable to resolve version for date-fns within >= 5.0
[02:14:39 ERR] Resolved version 1.20.0 satisfies [>= 1.0 < 2.0 && >= 1.20 < 2.0] but not >= 5.0
[02:14:39 ERR] Fable.DateFunctions -> Unable to resolve version for date-fns within >= 5.0
[02:14:39 ERR] Could not find a version that satisfies the required range

You might think that these logs are duplicates but actually the second error messages tells from which project the invalid version range is specified.

Developement dependencies

Now you can specify whether your npm dependencies should go in dependencies or devDependencies based on the attribute DevDependency in the XML tags:

<NpmDependencies>
  <NpmPackage Name="date-fns" Version=">= 1.0 lt 2.0" DevDependency="true" />
</NpmDependencies>

If the developer installed this package by mistake into dependencies it will be uninstalled and re-installed into devDependencies and vice-versa

General

The code probably needs cleanup and some refactoring, this is the first working version of these two features

MangelMaxime commented 5 years ago

I took the time to review the code but I did not test it yet.

MangelMaxime commented 5 years ago

You might think that these logs are duplicates but actually the second error messages tells from which project the invalid version range is specified.

Then we need to improve the log. Either by adding a "title/section" like Resolution result: or just remove the duplicated line.

The error message is easy to read and all the info are here.

[02:14:39 ERR] Fable.DateFunctions -> Unable to resolve version for date-fns within >= 5.0
[02:14:39 ERR] Resolved version 1.20.0 satisfies [>= 1.0 < 2.0 && >= 1.20 < 2.0] but not >= 5.0
[02:14:39 ERR] Could not find a version that satisfies the required range

But perhaps you added the "duplicate" for when having several "impossible package version to resolve"?

Zaid-Ajaj commented 5 years ago

But perhaps you added the "duplicate" for when having several "impossible package version to resolve"?

I left the "duplicate" error because it tells from which package the invalid package range is originating, for example the last two lines:

[02:14:39 ERR] {Library} -> Unable to resolve version for date-fns within >= 5.0
[02:14:39 ERR] Could not find a version that satisfies the required range

Tell you which {Library} was included that made the version invalid so it is extra helpful information IMO. We will have to see what users think of it

Zaid-Ajaj commented 5 years ago

Alright @MangelMaxime I have added a couple of issues regarding your review, one of which I will do myself later (refactoring ResolveAction see #21) and the other one to use Thoth can be added later.

For now I think this can be merged, right? (hopefully I did not change stuff you were working on)

MangelMaxime commented 5 years ago

the other one to use Thoth can be added later.

Yes, I will do it should not take too many times :)

For now I think this can be merged, right? (hopefully I did not change stuff you were working on)

Yes, please merge it. I didn't really test it but it's still in beta so that's fine :)

And also, it will allow me to check if we have conflicts or not.

MangelMaxime commented 5 years ago

Finally, I clicked on the merge button as it has the same effect if I do it or if you do it :p

And it allows me to have the code available sooner ^^

Zaid-Ajaj commented 5 years ago

Thanks a lot @MangelMaxime! looks like we are getting there...