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

KT-45789 Transitive npm dependencies #4888

Closed
wants to merge 2 commits into from

Conversation

mpetuska
Copy link
Contributor

Closes KT-45789

@ilgonmic
Copy link
Contributor

I am wondering what situation will occur, if direct and transitive dependency will be the same but different versions (with different cases when version ranges are compatible and incompatible).
It seems the most questionable in this issue

@mpetuska
Copy link
Contributor Author

Yeah, I wondered about that too. Shall I implement some form of resolution to guarantee uniqueness and implicity select the highest version?

Alternatively I could fail the build and report such occurences. Then the consumer would need to specify desired dependencies for clashes in direct npm dependencies at the sourceSet level

@mpetuska
Copy link
Contributor Author

mpetuska commented Jun 29, 2022

In any case, I think we can both agree that direct dependencies should take priority?

@mpetuska
Copy link
Contributor Author

To be clear, my first point was in context of version clashes between two transitive dependency sources, not between transitive and direct.

@mpetuska
Copy link
Contributor Author

How's this:

  1. If there's a direct version - use that and terminate resolution
  2. If there's a clash between two transitive versions, take higher and warn

@ilgonmic
Copy link
Contributor

Basically it is ok. But what if there will be versions like ^x.y.z and ~a.b.c. Do we consider such cases as necessary for resolution?

@mpetuska
Copy link
Contributor Author

I think it's fine to ignore version modifiers for resolution as long as a warning is printed. If there are any issues end user can just declare a direct dependency

@ilgonmic
Copy link
Contributor

Ok, as far as it is not widely announced. It looks good to me

@mpetuska
Copy link
Contributor Author

Do you think it should be toggled behind some feature flag? Or is it ok to just add it on top of existing workflow?

@ilgonmic
Copy link
Contributor

I think it is ok without feature flag, because it affects mainly experimental binaries.library()

@mpetuska
Copy link
Contributor Author

Ready for another look now.

@mpetuska
Copy link
Contributor Author

mpetuska commented Jul 4, 2022

A gentle reminder to have a look at this before I go ahead solving new merge conflicts :)

@ilgonmic
Copy link
Contributor

ilgonmic commented Jul 6, 2022

I’ll take a look, will try tomorrow

@ilgonmic
Copy link
Contributor

ilgonmic commented Jul 7, 2022

LGTM, I'll run it on CI

@ilgonmic
Copy link
Contributor

Merged in 3e6253c
Thanks!

@ilgonmic ilgonmic closed this Jul 11, 2022
@mpetuska mpetuska deleted the KT-45789 branch July 27, 2022 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants