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

Fix sourceMapURL when building bundles for windows/macOS #763

Closed
wants to merge 3 commits into from

Conversation

acoates-ms
Copy link
Contributor

Summary

The sourceMapUrl in bundles built for windows/macOS is of the format

//# sourceMappingURL=//localhost:8081/index.map?platform=windows&dev=true&hot=false&inlineSourceMap=false

This causes the chrome debugger to be unable to correctly load the source map. It turns out there is already code to modify this specifically for android/iOS. This change makes windows/macOS behavior match android/iOS.

See microsoft/react-native-windows#9407.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 25, 2022
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jan 25, 2022
@motiz88
Copy link
Contributor

motiz88 commented Jan 26, 2022

I'm trying to track down why we have this hack in the first place and whether it's necessary to enumerate these specific platforms in Metro core. (Platforms are configurable in Metro so any hardcoding of platform IDs is a code smell.)

Could we, for example: (ideas off the top of my head)

  • Infer the right protocol within Metro as-is? (Probably not - I'm guessing there's a legitimate use for the protocol: '' case)
  • Patch the RN Chrome debugger to fix up source map URLs before it evaluates the code?
  • Add a GET parameter to Metro specifically for forcing non-protocol-relative source map URLs? Then we can pass that parameter only from the Chrome debugger.

@acoates-ms
Copy link
Contributor Author

@motiz88 - Looks like vr went ahead and also added themselves to this hack. -- While I agree it would be better to fix it properly, can we at least get these main platforms in rather than blocking on cleaning this up?

@motiz88
Copy link
Contributor

motiz88 commented Nov 6, 2023

@acoates-ms I'll merge this, though I'm wary of shipping code we don't fundamentally understand. Can you please help with the followup here? In particular, it's not clear to me what you were referring to by "Chrome debugger" above - do you mean remote debugging (code executes in a browser) or direct debugging (code runs in RN's VM)?

@facebook-github-bot
Copy link
Contributor

@motiz88 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@acoates-ms
Copy link
Contributor Author

@acoates-ms I'll merge this, though I'm wary of shipping code we don't fundamentally understand. Can you please help with the followup here? In particular, it's not clear to me what you were referring to by "Chrome debugger" above - do you mean remote debugging (code executes in a browser) or direct debugging (code runs in RN's VM)?

Chrome debugger using CDP to direct debug against the VM in RN. -- I believe the comment added in the vr commit is incorrect, this is needed even now that we are not using remote debugging.

Its an onion of fixes needed to completely fix things though. Last time I looked at this, there was still an issue with chrome not loading the URL, even though it would now be a correct URL due to some sandboxing of requests allowed from the debugger context. - although if you run the chrome debugger in "node" debugging mode, then it would load the sourcemap with this fix.

@motiz88
Copy link
Contributor

motiz88 commented Nov 6, 2023

Yeah, we're aware of the issue with Chrome not fetching source maps over the network in general. There's a workaround in inspector-proxy that we're hoping to eventually replace with support for Network.loadNetworkResource in React Native.

For context: The workaround introduced a bug via accidental CDP message reordering, and proxying the fetch through the device would allow for distributed/tunnelled setups where the Metro URL known to the app isn't necessarily routable from where inspector-proxy is running.

@facebook-github-bot
Copy link
Contributor

@motiz88 merged this pull request in a510120.

robhogan pushed a commit that referenced this pull request Jan 9, 2024
Summary:
**Summary**

The sourceMapUrl in bundles built for windows/macOS is of the format

```
//# sourceMappingURL=//localhost:8081/index.map?platform=windows&dev=true&hot=false&inlineSourceMap=false
```

This causes the chrome debugger to be unable to correctly load the source map.  It turns out there is already code to modify this specifically for android/iOS.  This change makes windows/macOS behavior match android/iOS.

See microsoft/react-native-windows#9407.

Pull Request resolved: #763

Reviewed By: robhogan

Differential Revision: D51031030

Pulled By: motiz88

fbshipit-source-id: a325e417d725c53c5d96866d80f300a72389e4c2
@robhogan robhogan mentioned this pull request Jan 9, 2024
robhogan pushed a commit that referenced this pull request Jan 9, 2024
Summary:
**Summary**

The sourceMapUrl in bundles built for windows/macOS is of the format

```
//# sourceMappingURL=//localhost:8081/index.map?platform=windows&dev=true&hot=false&inlineSourceMap=false
```

This causes the chrome debugger to be unable to correctly load the source map.  It turns out there is already code to modify this specifically for android/iOS.  This change makes windows/macOS behavior match android/iOS.

See microsoft/react-native-windows#9407.

Pull Request resolved: #763

Reviewed By: robhogan

Differential Revision: D51031030

Pulled By: motiz88

fbshipit-source-id: a325e417d725c53c5d96866d80f300a72389e4c2
robhogan pushed a commit that referenced this pull request Jan 30, 2024
Summary:
**Summary**

The sourceMapUrl in bundles built for windows/macOS is of the format

```
//# sourceMappingURL=//localhost:8081/index.map?platform=windows&dev=true&hot=false&inlineSourceMap=false
```

This causes the chrome debugger to be unable to correctly load the source map.  It turns out there is already code to modify this specifically for android/iOS.  This change makes windows/macOS behavior match android/iOS.

See microsoft/react-native-windows#9407.

Pull Request resolved: #763

Reviewed By: robhogan

Differential Revision: D51031030

Pulled By: motiz88

fbshipit-source-id: a325e417d725c53c5d96866d80f300a72389e4c2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants