Skip to content

Conversation

@BrendonSled
Copy link
Contributor

Summary:

Very simple change.

Added the appId and appIdSuffix from the platform-android package. This allows anyone with an app suffix like .debug to profile hermes.

Test Plan:

This code is copy paste from the platform-android package.

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Thanks! Why not :)

@grabbou
Copy link
Member

grabbou commented Jul 6, 2021

Thanks, could you provide a use case for this addition?

At the same time, could you provide a description to our documentation? Simply open md file and add these options together with the description.

@evelant
Copy link

evelant commented Sep 6, 2021

@grabbou An example of when this is necessary: my app uses react-native-firebase, in development mode it connects to a separate staging firebase project. To do that it needs to have a different android package name than the production app, so I use --appIdSuffix=staging. Without this PR I don't think I can use profile-hermes because it won't pick up the correct package name in development mode and will then fail to copy the profile. I think this solves #1438

@BrendonSled
Copy link
Contributor Author

@grabbou this is a simple port of the same feature from the cli.
https://github.com/react-native-community/cli/blob/master/docs/commands.md#--appid-string

Copy link
Member

@grabbou grabbou left a comment

Choose a reason for hiding this comment

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

Generally looks good, but I do have a question (left a comment).

@BrendonSled
Copy link
Contributor Author

Thanks, could you provide a use case for this addition?

At the same time, could you provide a description to our documentation? Simply open md file and add these options together with the description.

For this I just copied this code from the cli

const packageNameWithSuffix = [appId || packageName, appIdSuffix]

This allows app suffix support. Example: com.test**.development**, com.test**.staging**,

@BrendonSled
Copy link
Contributor Author

Also @grabbou I would love to update the documentation for hermes. But I can't find the documentation for that specific cli.

The appidsuffix is already in the main cli commands, here:
https://github.com/react-native-community/cli/blob/master/docs/commands.md#--appidsuffix-string

Can you point me in the right direction?

@elizabeth-dev
Copy link

elizabeth-dev commented Oct 27, 2021

Also @grabbou I would love to update the documentation for hermes. But I can't find the documentation for that specific cli.

The appidsuffix is already in the main cli commands, here: https://github.com/react-native-community/cli/blob/master/docs/commands.md#--appidsuffix-string

Can you point me in the right direction?

@BrendonSled The documentation is in here, at the end of that file, you need to add two more paragraphs with the new options:

https://github.com/react-native-community/cli/blob/master/docs/commands.md#profile-hermes

I'm interested in this functionality too, so in case you are too busy, I'd have no problem in updating the docs on your fork if that's okay.

@henrymoulton
Copy link

@BrendonSled have just come across this issue/PR as I'm needing to do a bit of Hermes profiling in an app that has Debug/Staging/Production flavors.

@elizabeth-dev are you actively still needing this feature?

@thymikee / @grabbou anything needed to get this merged in?

@grabbou
Copy link
Member

grabbou commented Jan 24, 2022

The PR is ready to be merged, I have applied some tweaks. @henrymoulton would you mind testing that everything works as expected for you?

@grabbou grabbou merged commit 5de9156 into react-native-community:master Jan 24, 2022
@grabbou
Copy link
Member

grabbou commented Jan 24, 2022

Meanwhile, I have decided to ship it. Since we're not going to make a release anytime soon and this PR is additive, I will continue working on the CLI and wait for reports in the meantime.

@joaobertacchi
Copy link

@grabbou, @BrendonSled , I'm trying to use this new feature without success. Am I doing something wrong?

$ npx react-native@nightly profile-hermes --appId com.myapp ~/tmp
error: unknown option `--appId'

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants