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 #742

Closed
wants to merge 3 commits into from
Closed

Conversation

JunyiXie
Copy link

@@ -552,6 +552,36 @@ def add_resources(resource_file_references)
end
end

# remove on demand resource files to the resources build phase of the target.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit capital case 'R'

@dnkoutso
Copy link
Contributor

Thanks, can you please add tests and a changelog entry?

@dnkoutso
Copy link
Contributor

Please also run bundle exec rubocop -a to fix Rubocop issues for this to go green.

# @return [void]
#
def remove_on_demand_resources(on_demand_resource_file_references)
on_demand_resource_file_references.each do |file|
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is already defined? on_demand_resource_file_references?

Copy link
Contributor

Choose a reason for hiding this comment

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

nm I am stupid and this is a parameter. Hmmmm this doesn't really match what the function does. Its just removing resource file reference which can be done in remove_file_reference inside build_phase.rb. I don't think this method is really needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or at least can we assert that ASSET_TAGS is present on these resources?

Copy link
Author

Choose a reason for hiding this comment

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

emmm... Where do I write it more reasonable?

@shirakaba
Copy link

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

@JunyiXie I notice that this PR is waiting on further changes. If any help is needed with communication with @dnkoutso , I'd be happy to (try to) translate.

@JunyiXie 我注意到这个PR等一步改变。如果有任何沟通问题跟@dnkoutso,我愿意试试帮助翻译。

next if resources_build_phase.include?(file)
build_file = project.new(PBXBuildFile)
build_file.file_ref = file
build_file.settings = (build_file.settings ||= {}).merge({"ASSET_TAGS" => ["OnDemand"]})
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to be hard-coding asset tags here. According to Apple's docs tags are supposed to be manually specified, and those tags are then used in code to download the assets

Copy link
Author

Choose a reason for hiding this comment

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

thanks, now use the podspec on_demand_resources specified tag
81403b6#diff-5ee67a82fd5ae8aa882ddaf26476e138R577

@JunyiXie
Copy link
Author

I'm sorry I didn't read the GitHub news before, I will improve it in the next few days.

@JunyiXie
Copy link
Author

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

@JunyiXie I notice that this PR is waiting on further changes. If any help is needed with communication with @dnkoutso , I'd be happy to (try to) translate.

@JunyiXie 我注意到这个PR等一步改变。如果有任何沟通问题跟@dnkoutso,我愿意试试帮助翻译。

@JunyiXie JunyiXie closed this Apr 26, 2020
@JunyiXie JunyiXie reopened this Apr 26, 2020
@ignkarman
Copy link

Is there a plan to get this feature included in a future release? My team would love to make use of this functionality

@dnkoutso
Copy link
Contributor

@JunyiXie i would like to move this feature forward, can you rebase to latest master and add tests? if not I can take over this PR and give you credit for it so I can land it.

@dnkoutso
Copy link
Contributor

superseded by #844 due to inactivity. giving credit to original author as well.

@dnkoutso dnkoutso closed this Jul 23, 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

5 participants