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

Embedder script + local serve script #8

Merged
merged 12 commits into from
Mar 6, 2024
Merged

Embedder script + local serve script #8

merged 12 commits into from
Mar 6, 2024

Conversation

EdmondChuiHW
Copy link

@EdmondChuiHW EdmondChuiHW commented Mar 5, 2024

Summary

  • Add new embedderScript.js to entry point

Test plan

Test page load and the static files load in browser.

Upstreaming plan

  • This commit should be sent as a patch to the upstream devtools-frontend repo. I've reviewed the contribution guide.
  • This commit is React Native-specific and cannot be upstreamed.

@EdmondChuiHW EdmondChuiHW marked this pull request as ready for review March 5, 2024 15:59
Copy link

@motiz88 motiz88 left a comment

Choose a reason for hiding this comment

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

Recapping in-person discussion:

  • The Python script is redundant - we should instead develop with a local checkout of @react-native/dev-middleware (fbsource) and have it serve the devtools-frontend checkout.
  • Adding knowledge of the /debugger-frontend/ pathname to this repo is an antipattern. This repo should work just fine if served at whatever base URL.
  • We should not change the Content-Security-Policy for the sake of a testing script.
  • The README changes can mostly be reverted as a result of the above.
  • We should also rename static/embedderScript.js to something like embedder-static/main.js.

@EdmondChuiHW EdmondChuiHW requested a review from motiz88 March 5, 2024 17:06
Copy link

@hoxyq hoxyq left a comment

Choose a reason for hiding this comment

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

LGTM. Consider also adding async or defer attributes.

2 questions:

  1. Should it be strictly loaded before the entrypoint?
  2. Can user see 404 error somewhere in the logs / UI?

@EdmondChuiHW
Copy link
Author

  • Should it be strictly loaded before the entrypoint?

Good point. There's potential for Expo to add global variables/custom behaviour here. But for now our use case can support async (as we check readyState and listen to DOMContentLoaded already). Amended

  • Can user see 404 error somewhere in the logs / UI?

Yes it's visible in "dev tools in dev tools" console and network panels. Tho it has no impact on regular usage, I'll make a diff to maybe serve an empty file/status code by default to reduce the noise

image

@EdmondChuiHW EdmondChuiHW merged commit 8cc6f78 into facebookexperimental:main Mar 6, 2024
2 checks passed
@EdmondChuiHW EdmondChuiHW deleted the serve branch March 6, 2024 17:33
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.

4 participants