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

Add fabric on iOS #400

Merged
merged 33 commits into from
Aug 2, 2022
Merged

Add fabric on iOS #400

merged 33 commits into from
Aug 2, 2022

Conversation

okwasniewski
Copy link
Member

@okwasniewski okwasniewski commented Jul 19, 2022

Summary:

  • Setup codegen spec in flow
  • Move requireNativeComponent to separate file
  • Update podspec file
  • Add codegenConfig in package.json
  • Ensure backwards compatibility
  • iOS Implementation

Test Plan:

iOS: Install pods using RCT_NEW_ARCH_ENABLED=1 pod install

@BartoszKlonowski
Copy link
Member

This PR is part of #380 - linking this to the issue.

Copy link
Member

@BartoszKlonowski BartoszKlonowski left a comment

Choose a reason for hiding this comment

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

note: I've allowed myself to rebase your branch, as I saw there's a leftover in package lockfile with old npm version (your PR updates it correctly, but the updated version should be on main branch). I updated it on main and rebased your branch to avoid potential conflicts - sorry for inconvenience.

Copy link
Member

@BartoszKlonowski BartoszKlonowski left a comment

Choose a reason for hiding this comment

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

Looks like a decent base for further implementation 👍
I left some minor comments and we are missing some important props: trackImage, thumbImage and tapToSeek.

package/ios/RNCSliderComponentView.mm Show resolved Hide resolved
package/ios/RNCSliderComponentView.mm Show resolved Hide resolved
package/ios/RNCSliderComponentView.mm Outdated Show resolved Hide resolved
package/ios/RNCSliderComponentView.mm Show resolved Hide resolved
package/ios/RNCSliderComponentView.mm Outdated Show resolved Hide resolved
package/ios/RNCSliderComponentView.mm Outdated Show resolved Hide resolved
package/ios/RNCSliderComponentView.h Outdated Show resolved Hide resolved
package/ios/RNCSliderComponentView.h Outdated Show resolved Hide resolved
example/package-lock.json Outdated Show resolved Hide resolved
package/ios/RNCSliderComponentView.mm Outdated Show resolved Hide resolved
@okwasniewski okwasniewski changed the title Setup fabric on iOS Add fabric on iOS Jul 22, 2022
Copy link
Member

@BartoszKlonowski BartoszKlonowski left a comment

Choose a reason for hiding this comment

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

Just a couple of minor comments, a question and idea to bring back Pods caching.

.github/workflows/ReactNativeSlider-CI.yml Outdated Show resolved Hide resolved
.github/workflows/ReactNativeSlider-CI.yml Outdated Show resolved Hide resolved
.github/workflows/ReactNativeSlider-CI.yml Outdated Show resolved Hide resolved
.github/workflows/ReactNativeSlider-CI.yml Outdated Show resolved Hide resolved
.github/workflows/ReactNativeSlider-CI.yml Show resolved Hide resolved
.github/workflows/ReactNativeSlider-CI.yml Show resolved Hide resolved
device=$(xcrun simctl list devices "${device_name}" available | grep "${device_name} (")
re='\(([-0-9A-Fa-f]+)\)'
[[ $device =~ $re ]] || exit 1
xcodebuild -workspace example.xcworkspace -scheme example -destination "platform=iOS Simulator,id=${BASH_REMATCH[1]}" CODE_SIGNING_ALLOWED=NO COMPILER_INDEX_STORE_ENABLE=NO build
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering: Don't we have COMPILER_INDEX_STORE_ENABLED set to NO by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

* Bring back Pods to cache

* Use new-arch string in key for new arch cache

* Try to reinstall pods instead of creating new

* Separate deps installation between two caches

* Rename Pods reinstall step
Copy link
Member

@BartoszKlonowski BartoszKlonowski left a comment

Choose a reason for hiding this comment

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

Huge amount of hard work, looks good 👍
Thank you!

This check is no longer needed.
@okwasniewski okwasniewski merged commit 533db3c into chore/fabric-migration Aug 2, 2022
@okwasniewski okwasniewski mentioned this pull request Aug 4, 2022
okwasniewski added a commit that referenced this pull request Aug 8, 2022
* Add fabric on iOS (#400)

* Update podspec file for fabric

* Define codegen spec & move import to separate file

* Add codegenConfig in package.json

* Update podspec to detect .mm files

* Change spec types to floats

* Implement fabric component with props updating

* Re-order functions, add accessibilityIncrements prop

* Clean up imports, remove comment

* Sort out props updating, remove unused functions

* Add event handling

* Add tapToSeek implementation

* Fix tapToSeek

* Handle images using bridge

* Add missing typedef

* Verify the build for new arch with GH Actions

* Save new-arch Pods under new-arch cache key

* Use Podfile.lock with old arch and regenerate for new one

* Correct path for new arch Podfile.lock creation

* Disable flipper in Podfile

* Install pods with new arch flag

* Fix tapToSeek on iOS

* Allow value property to be controlled

* Generate project.pbxproj for new arch

* Separate npm & pods step in CI

* Change step names, fix cache keys

* Remove pods cache

* Run npm install if cache was not found

* Run build using xcodebuild

* Fix: Pods-related error after using Pods from cache (#407)

* Bring back Pods to cache

* Use new-arch string in key for new arch cache

* Try to reinstall pods instead of creating new

* Separate deps installation between two caches

* Rename Pods reinstall step

* Remove explicit folly version, default to old arch

* Add clean scripts in example for codegen cleanup

* Change CI step names

* Remove isFabricEnabled

This check is no longer needed.

Co-authored-by: BartoszKlonowski <Bartosz.Klonowski@callstack.com>

* Add fabric on Android (#402)

* Configure build.gradle

* Update libraryName on android

* Create ReactSlider shared implementation

* Split implementations into oldarch and newarch

* Dispatch events

* Cleanup eventDispatcher

* Make oldarch implementation use shared code

* Add defaults to js spec

* Clean up newarch ReactSliderManager

* Reorder props to fix disabled state

* Handle TestID setter

* Move ReactSliderShadowNode to shared implementation

* Share getExportedCustomDirectEventTypeConstants

* Remove comments, add empty line
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.

4 participants