Skip to content

feat: add link-assets command #2276

Merged
thymikee merged 50 commits intoreact-native-community:mainfrom
fabioh8010:feature/link-assets
Mar 29, 2024
Merged

feat: add link-assets command #2276
thymikee merged 50 commits intoreact-native-community:mainfrom
fabioh8010:feature/link-assets

Conversation

@fabioh8010
Copy link
Copy Markdown
Contributor

@fabioh8010 fabioh8010 commented Jan 23, 2024

Summary:

This PR aims to implement a feature to help users link their assets in their projects in an automated and efficient way. This PR will basically bring functionality from react-native-asset package + this new feature to link Android assets using XML Fonts. Due to the lack of maintenance in the original repo, we've published a new version under CK organisation here, but after some discussions we decided to bring the feature to the CLI.

Relevant links/discussions in chronological order:

  1. Original issue in Expensify repo
  2. CK proposal to implement the new feature in react-native-asset library
  3. PR with the new feature
  4. Issue about lack of maintenance and offer to help
  5. Decision to create a separate package under CK org, and proposal to move the feature to CLI
  6. Approval to move the feature to CLI

Tasks

  • Implementation of initial integration and commands
  • Migration and refactoring of original codebase to TypeScript
  • Support to handle Kotlin files
  • Unit tests
  • Final documentation

Test Plan:

TODO

Checklist

  • Documentation is up to date to reflect these changes.
  • Follows commit message convention described in CONTRIBUTING.md

@TMisiukiewicz
Copy link
Copy Markdown
Collaborator

Started code review and left some comments, gonna continue reviewing tomorrow :)

@TMisiukiewicz
Copy link
Copy Markdown
Collaborator

Just updated the code to handle Kotlin templates, tested it with freshly built app on 0.73 and seems to work well!

image

@fabioh8010
Copy link
Copy Markdown
Contributor Author

Thanks @TMisiukiewicz I will make these adjustments!

@thymikee
Copy link
Copy Markdown
Member

Ok I got a look at this PR and I have doubts exposing it to users. This commands works in a legacy way of manually changing Xcode and Gradle files – even asking users to do so. I hoped it would leverage the autolinking mechanism so that DX is nicer.

Please take a look at how rnx-kit team achieves that: microsoft/react-native-test-app#1828 in less than 200 lines of code.

If the time is pressure, I'm ok with merging that into the repository, but I'm not ok exposing this command to every user. It will add a lot of size to the package that's already pretty big. Therefore I'd suggest to document how one could use this package (npx @react-native-community/cli-link-assets instead of npx react-native link-assets) and iterate on a better solution that's aligned with autolinking and doesn't require developers to fiddle with native files.

@thymikee
Copy link
Copy Markdown
Member

Waiting for CI to be green and gonna merge it. Thanks so much for porting this code @fabioh8010, it's a great amount of effort to keep this library afloat and should be a good ground to iterate for a better, autolink-enabled solution in the future 👍🏼

@fabioh8010
Copy link
Copy Markdown
Contributor Author

@thymikee Could you run the E2E workflow tests again? Thanks!

@thymikee
Copy link
Copy Markdown
Member

why do I need to approve the pipeline twice, geez GitHub cmon

@thymikee thymikee merged commit 6c42d63 into react-native-community:main Mar 29, 2024
@thymikee
Copy link
Copy Markdown
Member

The CI passed 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation change feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants