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

[0.60.3] --sourcemap-output | enableHermes: false | gradle file | wrong output path #25693

Closed
HazAT opened this issue Jul 17, 2019 · 8 comments
Closed
Labels
Bug Resolution: Locked This issue was locked by the bot. Tech: Hermes Hermes Engine: https://hermesengine.dev/

Comments

@HazAT
Copy link

HazAT commented Jul 17, 2019

React Native version: 0.60.3

Steps To Reproduce

  1. react-native init test
  2. cd test/android
  3. Make sure you have these default settings:
project.ext.react = [
    entryFile: "index.js",
    enableHermes: false,  // clean and rebuild if changing
]
  1. ./gradlew assembleRelease --stacktrace
  2. Check content of android/app/build/generated/assets/react/release/index.android.bundle
  3. Check last line, content is: //# sourceMappingURL=index.android.bundle.packager.map

Expected:

//# sourceMappingURL=index.android.bundle.map


So the problem with this is at least twofold.
First I think if hermes is not enabled it shouldn't mess with the --sourcemap-output parameter at all. Reason why this came up is this: getsentry/sentry-react-native#612 (comment)
Hermes apparently needs an intermediate path and messes with this arg which breaks our setup.

At Sentry we try to pick up on this arg to determine the location but, this is not the real location since you move the file there later, see:

react-native/react.gradle

Lines 172 to 175 in 0190c9c

ant.move(
file: jsPackagerSourceMapFile,
tofile: jsOutputSourceMapFile
);

(Same for bundle)

And also, if the reference on the bottom of the file is not correct we cannot load it correctly.

I am not sure how to solve this since just copying instead of moving really doesn't.
If hermes is enabled it seems to work correctly, I am not sure why the source maps and bundle need to go in the intermediate folder tho.

If you know a way how to fix it, I would provide a PR for it.

@motiz88
Copy link
Contributor

motiz88 commented Jul 17, 2019

Hey, @HazAT, I'm sorry that this broke your workflow! Let's see what we can do.

if hermes is not enabled it shouldn't mess with the --sourcemap-output parameter at all.

This was a conscious choice, intended to keep things consistent for users of the two JS engines. The idea is that you will always find the source map at the same location, relative to the bundle file, whether you're using JSC or Hermes. (For context: Previously the Android build did not generate source maps at all.)

If you know a way how to fix it, I would provide a PR for it.

This would be massively appreciated - thank you!

My suggestion is to change react.gradle to pass a different source map output path to the bundling command, depending on whether Hermes is enabled:

  • For Hermes, pass jsPackagerSourceMapFile as we do now.
  • For JSC, pass jsOutputSourceMapFile and don't do any moving/renaming afterwards.

That should result in the correct source map path being written to the JS bundle. Would that help your use case?

Note that Hermes support isn't on master yet, so if you do start working on a PR you should base it on the 0.60-stable branch until #25613 lands.

Please let me know whether I can help with anything else.

@HazAT
Copy link
Author

HazAT commented Jul 17, 2019

@motiz88 Thanks for the explanation, I will try to fix this in a PR, will reference you.

@react-native-bot react-native-bot added the Tech: Hermes Hermes Engine: https://hermesengine.dev/ label Jul 17, 2019
@HazAT
Copy link
Author

HazAT commented Jul 18, 2019

@motiz88 I've created this PR #25700 tested it locally and it works.
In terms of:

if hermes is not enabled it shouldn't mess with the --sourcemap-output parameter at all.

This was a conscious choice, intended to keep things consistent for users of the two JS engines. The idea is that you will always find the source map at the same location, relative to the bundle file, whether you're using JSC or Hermes. (For context: Previously the Android build did not generate source maps at all.)

One thing you should consider is that this forcefully now creates a source map for all builds and if ppl don't delete the source map before creating the APK, the source map will be shipped with it.
This will significantly increase the APK size.

In our SDK we cleanup the source map afterward so it will not be bundled:
https://github.com/getsentry/sentry-react-native/blob/ba01716f45fe40185f7785e064dcd0ed685fe417/sentry.gradle#L116-L121
but for those ppl who don't use Sentry, it will be bundled.

So, if you reconsider your choice of doing that, I can change the PR to not forcefully set the --sourcemap-output parameter.

@motiz88
Copy link
Contributor

motiz88 commented Jul 18, 2019

The intention was never to bundle the source map into the APK; if that's what we're doing, it's a bug. We might need to put the source map in a different directory to avoid this. However, we do want the release build to generate a source map by default.

@motiz88
Copy link
Contributor

motiz88 commented Jul 18, 2019

@HazAT, could you take a look at #25710? Is this a viable setup for your use case?

@HazAT
Copy link
Author

HazAT commented Jul 18, 2019

@motiz88 Will try to run it an let you know!

@HazAT
Copy link
Author

HazAT commented Jul 19, 2019

@motiz88 confirmed, it works! Thx!

@motiz88
Copy link
Contributor

motiz88 commented Jul 19, 2019

Thanks again for surfacing this and sending a PR! Feel free to tag me on any further issues/requests related to source maps or crash logging.

@motiz88 motiz88 closed this as completed Jul 19, 2019
@facebook facebook locked as resolved and limited conversation to collaborators Oct 7, 2021
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Oct 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Resolution: Locked This issue was locked by the bot. Tech: Hermes Hermes Engine: https://hermesengine.dev/
Projects
None yet
Development

No branches or pull requests

3 participants