Skip to content

Fix Hermes enabled check for iOS#1756

Merged
DmitriyKirakosyan merged 1 commit intomicrosoft:masterfrom
bryansum:bryan/fix-hermes-enabled-check
Dec 9, 2021
Merged

Fix Hermes enabled check for iOS#1756
DmitriyKirakosyan merged 1 commit intomicrosoft:masterfrom
bryansum:bryan/fix-hermes-enabled-check

Conversation

@bryansum
Copy link
Copy Markdown
Contributor

@bryansum bryansum commented Dec 7, 2021

Noticed that despite Hermes being enabled I wasn't seeing the appcenter-cli try to create Hermes bytecode for the build -- this was because the check for Hermes was too specific -- it expects:

:hermes_enables => true

when equally valid is new Ruby syntax:

hermes_enabled: true

This change adds the latter case as well to the test.

@DmitriyKirakosyan
Copy link
Copy Markdown
Contributor

Hi @bryansum , thank you for your contribution!

Could you please link the source where this :hermes_enables => true -> hermes_enabled: true replacement was introduced? I'm still seeing the old syntax in react-native docs.

@bryansum
Copy link
Copy Markdown
Contributor Author

bryansum commented Dec 7, 2021

It hasn't been replaced anywhere, it's just that that syntax is also valid Ruby, so it would seem worthwhile covering this case as well. For instance my Podfile looks like:

  use_react_native!(
    path: config[:reactNativePath],
    # to enable hermes on iOS, change `false` to `true` and then install pods
    hermes_enabled: true
  )

and indeed has Hermes enabled.

Generally speaking, this method of detection seems generally a bit brittle in any case -- what might make more sense would be to instead read the Podfile.lock and look for the hermes-engine string, since that's what the hermes_enabled flag enables anyway: https://github.com/facebook/react-native/blob/main/scripts/react_native_pods.rb#L85 I can make that change too if that makes more sense.

@DmitriyKirakosyan
Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@DmitriyKirakosyan
Copy link
Copy Markdown
Contributor

DmitriyKirakosyan commented Dec 9, 2021

Hi @bryansum , since this is a utility method and it can be used anywhere, I would like to avoid relying on a generated file which may not exist in project at some point.
Let's keep this changes at this moment, and if you still feel this check should be done in lock file, please create a separate PR or issue and we will have a deeper look into it.
Thank you again for the contribution!

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.

3 participants