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

Combine package.json and target/frontent/package.json #7018

Closed
caalador opened this issue Nov 25, 2019 · 7 comments · Fixed by #7056
Closed

Combine package.json and target/frontent/package.json #7018

caalador opened this issue Nov 25, 2019 · 7 comments · Fixed by #7056

Comments

@caalador
Copy link
Contributor

We should make package.json fully generated by flow and move the information in target/frontend/package.json into package.json so that the plugin handles all dependencies in one place.

This would mean that any user modification directly on the package.json file might be lost on an update by flow, but it would make the developers life easier as then the knowing which modules the project is actually depending on is easier and keeping track on versions can simply be done in version control.

Also this will simplify our code as we do not need to do hashes for dependencies to know if we need to run npm i after a mvn clean

Part of #6966

@caalador caalador added this to Inbox - needs triage in OLD Vaadin Flow ongoing work (Vaadin 10+) Nov 25, 2019
@Artur-
Copy link
Member

Artur- commented Nov 25, 2019

This would mean that any user modification directly on the package.json file might be lost on an update by flow

In which case would something be lost? Shouldn't Flow know which parts it "owns" and which parts is owned by the user?

@caalador
Copy link
Contributor Author

Dependencies are removed if they are not in the found dependencies map (e.g. all user changes).
It was proposed to have a flag for cleaning extra dependencies automatically by default and having the possibility of having the framework only add dependencies which would make the package.json user editable for all parts except for any annotated packages. (that's basically true also now as version collisions will probably break the build)

In the case where the user doesn't want auto removal they would be responsible for fixing any typos as highlighted by Manolo the comment in #6966

@Legioth
Copy link
Member

Legioth commented Nov 26, 2019

Would this mean that if read some guide online and blindly follow the instructions to run npm install --save left-pad, then my dependency will be removed the next time I start the server?

@caalador
Copy link
Contributor Author

By default it would, as it wouldn't be found on the java side. Or then the default should be only add, but then any typos in annotations would not be fixed in the package.json making the user need to fix the dependencies.

@Legioth
Copy link
Member

Legioth commented Nov 26, 2019

Let me propose a slightly more complicated design that would work in all cases that I'm aware of.

Introduce a vaadinDependencies field in package.json that contains a copy of anything that Vaadin as added as a regular dependencies. Based on this, Vaadin is only allowed to remove (or update) values in dependencies if they still match the entry in vaadinDependencies. The structure of vaadinDependencies can either be exactly the same JSON structure as for regular dependencies, or alternatively encoding that JSON as base64 to discourage users from eidting it manually.

In this way, something that you add manually using e.g. npm install --save left-pad will never be removed by Vaadin. At the same time, adding @NpmPackage(value="left-bad", version="1.3.0") would cause a corresponding entry to be added also to vaadinDependencies which means that once the typo is fixed, Vaadin would see that there's a Vaadin-managed left-bad dependency but no corresponding @NpmPackage annotation. Based on this, the bad dependency could be safely removed.

If the user has manually updated a version number for a dependency that was originally added by Vaadin, then the entry in vaadinDependencies would not match and the dependency would thus not be removed automatically. If the user then updates the version number in @NpmPackage to a version newer than the one manually defined in package.json, then Vaadin would log a warning about a potential version mismatch during startup.

@caalador
Copy link
Contributor Author

caalador commented Nov 26, 2019

This would be acceptable overhead, and still a lot simpler than the current setup.
To also satisfy #6907 at the same time would be to add the full object

"vaadin": {
  "dependencies": {},
  "devDependencies": {}
}

Then we could accept manual changes to devDependencies also with minimal changes in functionality.

@caalador
Copy link
Contributor Author

caalador commented Nov 27, 2019

Acceptance criteria:

  • target/frontend/package.json is removed and all dependencies are added to package.json
  • package.json gets a new JsonObject vaadin that contains dependencies and devDependencies that are the framework managed packages.
  • managed packages in vaadin should accept newer versions by users to dependencies, but override too old versions
  • Framework should not touch dependencies that are not found in vaadin dependencies.

@caalador caalador moved this from Inbox - needs triage to Product backlog in OLD Vaadin Flow ongoing work (Vaadin 10+) Nov 27, 2019
@mehdi-vaadin mehdi-vaadin moved this from Product backlog to Ready To Go in OLD Vaadin Flow ongoing work (Vaadin 10+) Nov 27, 2019
@caalador caalador self-assigned this Nov 27, 2019
@project-bot project-bot bot moved this from Ready To Go to In progress in OLD Vaadin Flow ongoing work (Vaadin 10+) Nov 27, 2019
caalador added a commit that referenced this issue Nov 28, 2019
Combine main- and appPackage.json
into one package.json.
Handle framework dependencies so that
user can add their own or override versions
with newer ones.

Fixes #7018
Closes #6907
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from In progress to Done - pending release Dec 3, 2019
denis-anisimov pushed a commit that referenced this issue Dec 3, 2019
Combine main- and appPackage.json
into one package.json.
Handle framework dependencies so that
user can add their own or override versions
with newer ones.

Fixes #7018
Closes #6907
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Development

Successfully merging a pull request may close this issue.

3 participants