flow icon indicating copy to clipboard operation
flow copied to clipboard

Versions in package.json and package-lock.json files gets rewritten randomly

Open SomMeri opened this issue 3 years ago • 3 comments

Description of the bug

Issues There are multiple related issues with the same root cause. Issues:

  1. Sometimes when our projects project starts, the version of date-fns library in package.json and package-lock.json gets downgraded. It happens unpredictably - sometimes the library gets downgraded and sometimes it stays the same. Most of the time it stays the same.
  2. When user upgrades/downgrades npm library used by vaadin itself using @NpmPackage annotation, the actual library version in the package.json after startup is unpredictable. Sometimes it is user upgrades/downgrades one, other times it is the version from vaadin.
  3. When project includes .jar library and it contains @NpmPackage with version we do not want, the vaadin will rewrite package.json and package-lock.json all the same. There is no way to actually fix the version of the library inside our project.

Root cause: When project classpath contains multiple @NpmPackage annotations of the same library with different versions, the package.json and package-lock.json are rewritten with random version on startup. There is no warning or error in log. This makes it impossible to reliably rewrite version of vaadin npm library - vaadin classes are on the same classpath and treated the same way as annotations in our project.

Configuration: Vaadin / Flow version: 22.0.20 date-fns js lib version in vaadin: 2.28.0

Extract of our package.json - _date-fns is has 2.28.0 by default:

{
  "dependencies": {
 
      [...]
    "date-fns": "2.28.0",
      [...]
  },
  "vaadin": {
    "dependencies": {
      [...]
      "date-fns": "2.28.0",
      [...]
    },
  }
}

Our custom class (it came from .jar made by somebody else, I can not easily change the NpmPackage)

@JsModule("./date-picker/date-picker-pattern.js")
@NpmPackage(
  value = "date-fns",
  version = "2.23.0"
)
public class CustomDatePicker extends DatePicker{
 // ...
}

What happens inside vaadin:

  1. FullDependenciesScanner picks up 4 versions of date-fns lib:
      - date-fns 2.23.0 com.us.CustomDatePicker
      - date-fns 2.28.0 com.us.CustomDatePicker // this is inherited from vaadin class
      - date-fns 2.28.0 com.vaadin.flow.component.datepicker.DatePicker
      - date-fns 2.28.0 com.vaadin.flow.component.datetimepicker.DateTimePickerDatePicker
    
  2. FrontendDependenciesScanner then picks random of these four entries to return from its getPackages method. It can be either 2.23.0 or 2.28.0.
  3. If the scanner picks 2.23.0, then the version in package.json gets replaced. This happens in NodeUpdater. image

Expected behavior

  1. The package.json and package-lock.json should be stable. They should not change between two runs of the same project.
  2. It should be possible to predictably override js library version and solve versioning conflicts.
  3. If nothing else, vaadin should output some warning into log/console so that developer know there is versioning conflict on the classpath.

Minimal reproducible example

Place class like this into vaadin project

@NpmPackage(
  value = "date-fns",
  version = "2.23.0"
)
public class CustomDatePicker extends DatePicker{
 // ...
}

Versions

  • Vaadin / Flow version: 22.0.20
  • Java version: 1.8
  • OS version: Windows 10

SomMeri avatar Aug 18 '22 17:08 SomMeri

A possible fix could be to introduce some kind of ordering for NpmPackage, where the default of Vaadin uses e.g. -1 or some other low number and the user can add it's own ordering with higher precedence. The same is done with spring boot's configuration and there it is a well known mechanism in the industry and shouldn't be to hard to implement.

Edit: interesting, the NpmPackage javadocs says "Troubleshooting: when two or more annotations with the same package value are found in the class-path, and their versions do not match the build process will fail." - looks like this is not the case.

knoobie avatar Aug 18 '22 17:08 knoobie

Interesting, based on that javadoc it should just fail. But this could cause issues when using libraries - if two .jars included slightly different version, it would be impossible to use both and solve the conflict by hand.

In general, I would expect the package-lock.json be the place where the project can set its own version. I would expect vaadin to not overwrite it at all. Then, project could place whatever they want in it.

I think they are rewriting package lock so that vaadin can upgrade its own js libraries as it raises versions. And that the vaadinVersion.isEqualTo(packageVersion) is sort of heuristic to check whether the version came from vaadin itself or not.

SomMeri avatar Aug 18 '22 18:08 SomMeri

Since Vaadin 23 class scanning order is predictable in development mode: classes to inspect are sorted by name (Class.getName()) and @NpmPackage annotations are processed top-down according to the class hierarchy, so annotation from parent classes takes precedence. When different values are found, a warning is logged

WARN com.vaadin.flow.server.frontend.scanner.FullDependenciesScanner - Multiple npm versions for date-fns found:  [2.28.0, 2.23.0]. First version found '2.28.0' will be considered.

However, on production build, it seems like classes are not sorted by name (see ReflectionsClassFinder class), so it may be possible to have unpredictable results.

So, as a summary, since Vaadin 23:

  • presence of duplicated NPM packages is logged (Point 3 of expected behavior)
  • In development mode, class scanning order is predictable and package.json is stable (Point 1 of expected behavior)
  • AFAIK package-lock.json is never written by Flow
  • detected version may be different in production build and development mode
  • NpmPackage.version javadocs are misleading

Action we can take immediately

  • ReflectionsClassFinder should sort classes to scan (getAnnotatedClasses()) the same way as DefaultClassFinder to uniform dev mode and production build
  • NpmPackage.version javadocs should be updated to describe current behavior in case of multiple versions

Having a way to set a priority on @NpmPackage may deserve a separated ticket and discussion

mcollovati avatar Sep 16 '22 07:09 mcollovati