-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Soft dependency support for resources #2920
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
Conversation
Adds support for soft dependencies, indicated by the manifest entries `soft_dependency` and `soft_dependencies`. Soft dependencies only get started if they exist, making it possible to add optional dependencies to a resource.
|
Okay I wanted to rename the branch on my repo, but github doesn't seem to like it. |
|
Thank you for the contribution! I understand the motivation behind this PR and the code looks reasonable. But I don't have enough context to make the final call on if it's actually needed. @prikolium-cfx WDYT? |
Should be discussed together with team. Can't say right now pros and cons of such decision |
Adds single quotes around resource names in dependency loading traces to clarify what is part of the resource name.
|
Sounds good! I added some quotes around the resource names to clarify what is and what is not part of the resource names. This also makes it match the other traces in the dependency loader. |
|
@prikolium-cfx any updates on this? |
| if (!other.GetRef()) | ||
| { | ||
| trace("Could not find dependency %s for resource %s.\n", dependency.second, resource->GetName()); | ||
| trace("Could not find %s '%s' for resource '%s'.\n", soft ? "soft dependency" : "dependency", dependency.second, resource->GetName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we notify user about missed soft dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So they know why something is not working even though they think it will. Say your resource uses ox_target and registers eye options if it's present. If it has been renamed or something users might expect it to still work but then they can see this message and investigate.
Normal depencies also log when they don't get found and then it proceeds to cancel starting the resource. This is pretty much the same except that it doesn't cancel starting the resource, but still logs that it's missing something.
| if (!success) | ||
| { | ||
| trace("Could not start dependency %s for resource %s.\n", dependency.second, resource->GetName()); | ||
| trace("Could not start %s '%s' for resource '%s'.\n", soft ? "soft dependency" : "dependency", dependency.second, resource->GetName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment. Here it's even more important to log. If a soft dependency is present people will expect it to work, but if it can't be started for some reason they should be informed about that.
|
|
||
| // Load soft dependencies defined with `soft_dependency` or `soft_dependencies`. | ||
| // We ignore the result, as we don't want to cancel loading the resource if a soft dependency is missing. | ||
| loadDeps("soft_dependency", true) && loadDeps("soft_dependencie", true); // soft_dependencies without s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please leave only 1 way of setting soft dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same way but the plural form. Just like is done for regular dependencies. You've got dependency and dependencies. Normally you use server_script and it would append an s, resulting in the correct server_scripts. But since the plural of dependency is dependencies, we need to use dependencie as well.
This same principle was applied here for soft_depenency and soft_dependencie resulting in soft_dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same way but the plural form. Just like is done for regular dependencies. You've got
dependencyanddependencies. Normally you useserver_scriptand it would append an s, resulting in the correctserver_scripts. But since the plural of dependency is dependencies, we need to usedependencieas well.This same principle was applied here for
soft_depenencyandsoft_dependencieresulting insoft_dependencies.
To be honest today we discussed with team this naming and soft_dependency doesn't make much sense and possible name for that field that we found would be load_after so it's not confusing creators. Probably you can suggest better name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
load_after makes very little sense to me as it just implies that the resource should load after another one. The whole point of this PR is to introduce a soft dependency as is used in many other types of software. It also starts the soft dependency if it is found while load after implies that it starts this one if and only if the other one is already started. That's not what this feature does so it makes very little sense to me to name it that.
I think load_after would create more confusion for developers. Most developers should already be familiar with how soft dependencies work as well for example from npm packages, php or minecraft plugins. But also many many other software systems.
Could you explain why it doesn't make much sense to you? As a software engineer I've seen this type of dependency used a lot.
Here is a link to how it works in magento, maybe that makes it more clear for you: https://developer.adobe.com/commerce/php/architecture/modules/dependencies/
Please also take the time to read the full proposal in the forum post, as it explains way more than what I've said here: https://forum.cfx.re/t/soft-dependencies-for-resources/5281288/1
|
Hey @prikolium-cfx @Nobelium-cfx and the rest of the team! Just checking in since it's been a while. Is this ever going to get merged? |
Current manifest system is already overcomplicated and such change will make it more confusing. Thanks for your efforts, but we won't accept such change. |
That's a shame, I feel like it would make it a lot easier for developers. Why do you think adding this would make it more confusing? |
Goal of this PR
This PR adds support for soft dependencies, indicated by the manifest entries
soft_dependencyandsoft_dependencies. Soft dependencies only get started if they exist, making it possible to add optional dependencies to a resource. This is useful when a resource can work fine without a dependency, but can benefit from having access to that dependency.See the proposal on the forum for more details: https://forum.cfx.re/t/soft-dependencies-for-resources/5281288/1
How is this PR achieving the goal
This PR implements soft dependencies by loading them the same way as normal dependencies, but if one is missing or fails to start it skips it and continues to the next one. It also still prints the same information as with regular dependencies, and only tries to load the soft dependencies after the hard dependencies have been checked and loaded to prevent starting them when the resource can't start anyway.
This PR specifically adds the
const bool soft = falseparameter to theloadDepsfunction. The paramter has a default value of false, to remain backwards compatible. Whensoftis set totruethe following things are different as opposed to loading hard dependencies:falseearly to stop the resource from loading.resourceDependants, as stopping the soft dependency should not prevent the resource from running.falseearly to stop the resource from loading and storing the dependency inresourceDependenciesto start it later.This PR applies to the following area(s)
This applies to the server and clients. The changes were made in
citizen-resources-core.Successfully tested on
The changes were tested on the server and FiveM client.
Game builds: b3095
Platforms: Windows
Checklist
Fixes issues
None.