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 On-Demand Resource #622

Closed
wants to merge 4 commits into from
Closed

Conversation

JunyiXie
Copy link

@shirakaba
Copy link

shirakaba commented Apr 25, 2020

To provide more context: @JunyiXie has written an excellent blog post summarising the changes necessary to CocoaPods/Core (this PR) and CocoaPods/Xcodeproj (CocoaPods/Xcodeproj#742) in order to support On-Demand Resources. It's quite understandable with Google Translate.

I'm looking forward to this PR!

# paths of the on_demand_resources to copy and the values the paths of
# the on_demand_resources that should be copied.
#
spec_attr_accessor :on_demand_resources
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would want this to be a Hash that allows Pod authors to specify the tag (see my comment here)

example:

s.on_demand_resources = {
    'Tag1' => ['file1.png', 'file2.png']
}

we also may need to provide a way for Pod authors to specify the category of the tag. The category determines how the resource gets fetched. To quote the docs:

Initial install tags. The resources are downloaded at the same time as the app. The size of the resources is included in the total size for the app in the App Store. The tags can be purged when they are not being accessed by at least one NSBundleResourceRequest object.

Prefetch tag order. The resources start downloading after the app is installed. The tags will be downloaded in the order in which they are listed in the Prefetched tag order group.

Dowloaded only on demand. The tags are downloaded when requested by the app.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea. I will refer to your design for implementation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implementation f240511

@amorde
Copy link
Member

amorde commented Apr 25, 2020

Thanks for working on this! I think there's some additional questions we need to answer before this can land, but really appreciate you taking the time to investigate this, and @shirakaba thank you for helping facilitate review

@JunyiXie
Copy link
Author

Thanks for suggestions, I modified the implementation of podspec dsl. @amorde

@amorde
Copy link
Member

amorde commented May 16, 2020

Commenting to say I haven't forgotten about this! Have been taking a break, need a bit more time to review. Thank you for your patience 🙏


* Support On-Demand Resource
[JunyiXie](https://github.com/JunyiXie)
[#issue_number](https://github.com/CocoaPods/Core/pull/622)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make this 622 instead of #issue_number

@@ -1370,6 +1390,7 @@ def dependency=(args)
:file_patterns => true,
:singularize => true


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unneeded?

#
# @return [Hash] the resources.
#
def _prepare_on_demand_resources(value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please add a test for this logic?

@dnkoutso dnkoutso added this to the 1.11.0 milestone Jul 21, 2021
@dnkoutso
Copy link
Contributor

superseded by #690

@dnkoutso dnkoutso closed this Jul 24, 2021
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.

None yet

4 participants