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

Support LocationFallback on ILinkResources #728

Merged
merged 12 commits into from
Aug 16, 2019
Merged

Conversation

cafour
Copy link
Contributor

@cafour cafour commented Jul 24, 2019

Fixes #715.

This PR adds support for LinkResourceBase.LocationFallback during URL building and resolution of ILinkResources by adding a location query parameter to the dotvvmResource route.


To illustrate the problem this PR solves, let's have the following resource registration:

resources.Register("FeatureSamples_Resources_LocationFallback", new ScriptResource {
    Location = new FileResourceLocation("~/Scripts/testResource_failsToLoad.js"),
    LocationFallback = new ResourceLocationFallback(
        "window.isTestResourceLoaded",
        new FileResourceLocation("~/Scripts/testResource_locationFallback.js"))
});

Without the query parameter, the generated URL of the fallback location is the same as of the primary location and thus it has no effect.


I realize the main purpose of the LocationFallback property is having a backup plan for when a CDN is unavailable, and that a FileResourceLocation as fallback for another FileResourceLocation seems a little nonsensical but it is a valid configuration if only for testing the LocationFallback feature.

@cafour cafour requested review from exyi, quigamdev and tomasherceg and removed request for exyi and quigamdev July 24, 2019 10:28
@exyi
Copy link
Member

exyi commented Jul 29, 2019

Is there any practical need for the location distinction? Surprisingly having multiple FileResourceLocations does not make much sense, since this fallback feature is designed to handle a failure to load a 3rd party resource. If we want to support failures to load local resource (due to missing file) I'd prefer to handle that server-side and not annoy the client-side about such issue.

@cafour
Copy link
Contributor Author

cafour commented Jul 29, 2019

The feature may have been designed to handle a failure in loading of a 3rd party resource but this intention is not enforced. The way it's designed (looking at the fallback condition) means it can be used to handle errors in a piece of javascript code. Having a fallback in that case could be useful.

Personally, I think we should support this scenario. But if we decide not to, I think, at the very least, we should notify the user that having multiple FileResourceLocations is meaningless.

(There's also my need to test LocationFallback in order to fix #702, but that can be solved another way.)

@cafour cafour requested a review from exyi August 14, 2019 11:44
@cafour
Copy link
Contributor Author

cafour commented Aug 16, 2019

Merging after discussion with @exyi.

@cafour cafour merged commit 41a1eeb into master Aug 16, 2019
@cafour cafour deleted the fix/fallback-location branch August 16, 2019 11:33
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.

LocationFallback of a resource doesn't get rendered
2 participants