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

Update flowless; Exclude transitive ReactFX dependency explicitly #445

Merged
merged 2 commits into from
Mar 3, 2017

Conversation

JordanMartinez
Copy link
Contributor

No description provided.

@JordanMartinez JordanMartinez merged commit 0668ae8 into FXMisc:master Mar 3, 2017
@JordanMartinez JordanMartinez deleted the updateFlowless branch March 3, 2017 01:59
@TomasMikula
Copy link
Member

Did the transitive dependency cause problems?

@JordanMartinez
Copy link
Contributor Author

I'm not sure. It did when we used a newer version of ReactFX before in RichTextFX but not in Flowless or UndoFX. It might not now, but I haven't tested it without it.

@JordanMartinez
Copy link
Contributor Author

Regardless, I thought it would be better to exclude ReactFX specifically rather than using the global "exclude all transitive dependencies" option in case one of those dependencies should add another dependency.

@TomasMikula
Copy link
Member

Isn't it a feature to get an error when there are incompatible versions of a dependency?

@JordanMartinez
Copy link
Contributor Author

uh..... I don't know....?

@TomasMikula
Copy link
Member

I find it better than at runtime. And when that happens, you can then choose to manually override the version of the dependency, on a case by case basis.

@JordanMartinez
Copy link
Contributor Author

I find it better than at runtime. And when that happens, you can then choose to manually override the version of the dependency, on a case by case basis.

Is there an article that explains this more? I'd hate to waste your time by asking you to explain it further.

Also see #446

@TomasMikula
Copy link
Member

My reasoning is that most of the time the build with transitive dependencies will be fine. In the rare case of conflicting dependency versions, I find it better to know about it. In case the transitive dependency is newer than the direct dependency, we would probably want RichTextFX to move to the newer version as well, rather than silently override the dependency version to the older one (which might cause problems for the library that asks for the newer version).

@JordanMartinez
Copy link
Contributor Author

Ah ok. Now I see what you mean. In my own words, you want to know when a transitive dependency has a version conflict so we know how to resolve that conflict: if the transitive dependency is an older version, then we should update it asap (since we only use your projects as dependencies), and if it's a newer one, it simply informs us to also update RichTextFX's version.

ReactFX was excluded at first because we were using a snapshot version (or something like that). I think we (or I) forgot to update it to a stable release once that was resolved.

@TomasMikula
Copy link
Member

Yes, and if we had a dependency that we don't control that requires an older version of a transitive dependency, then we could choose to temporarily exclude that transitive dependency, but update the build as soon as the versions are in sync again.

@JordanMartinez
Copy link
Contributor Author

Shouldn't this guideline be added to Contributing.md?

@TomasMikula
Copy link
Member

Naah, let's not overload contributors with too much bureaucracy :)

@JordanMartinez
Copy link
Contributor Author

I see your point. On the other hand, you could just assign this responsibility to me. That wouldn't overload other (or future) contributors and would still insure it gets done. If I should stop contributing to this project, you'd be in the same place as we are now if you decide not to do so. The pros outweigh the cons in my mind, but I'll leave that up to you to decide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants