-
-
Notifications
You must be signed in to change notification settings - Fork 638
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
Use absolute URL for webview baseUrl
on iOS.
#4183
Conversation
src/webview/MessageList.js
Outdated
// | ||
// [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Resource.20URLs/near/926650 | ||
// [2] https://github.com/jsdom/whatwg-url/tree/v7.0.0/#specification-conformance | ||
baseUrl.origin === new WhatwgURL('file:///').origin ? 'file://' : baseUrl.origin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right—in this revision, I've acted on what I said here and abandoned ever pointing to what's served by the packager.
In effect, this means the URL should always start with file://
—so we could still just say "file://", if that's preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just about to ask what this code was doing and why this part became more complicated 🙂 I guess that's the answer!
Yeah, let's leave this in the simple form it's in until/unless we find a need to do something more complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chrisbobbe ! Looks good apart from the comment above and a few comments below.
src/webview/MessageList.js
Outdated
@@ -309,8 +285,16 @@ class MessageList extends Component<Props> { | |||
// https://github.com/react-native-community/react-native-webview/pull/697 | |||
return ( | |||
<WebView | |||
source={{ baseUrl, html }} | |||
originWhitelist={['file://']} | |||
source={{ baseUrl: baseUrl.toString(), html }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I get a Flow error on this line!
Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/webview/MessageList.js:288:17
Could not decide which case to select. Since case 1 [1] may work but if it doesn't
case 2 [1] looks promising too. To fix add a type annotation to call of method
toString [2].
src/webview/MessageList.js
285│ // https://github.com/react-native-community/react-native-webview/pull/697
286│ return (
287│ <WebView
[2] 288│ source={{ baseUrl: baseUrl.toString(), html }}
289│ originWhitelist={[
290│ // `baseUrl.origin` may be "null" (an "opaque origin") for
291│ // `file://` URLs. [1] [2] `react-native-webview` doesn't
flow-typed/react-native-webview_v8.x.x.js
[1] 495│ source?: WebViewSource,
Found 1 error
Puzzling that I see that even though it seems to have passed in CI. If you're not seeing the error, let's debug in chat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems to have passed in CI
Ah, I think CI isn't a thing anymore, hence #4174?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah indeed 🙂 (and fixed now.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the fix was:
source={{ baseUrl: (baseUrl.toString(): string), html }}
i.e. adding an explicit cast to string
. Weird that that makes a difference -- this method's return type is shown as simply string
already. Seems like a Flow bug.
But the behavior with that workaround seems like the right behavior, so 🤷
914c02e
to
e99c8de
Compare
Just pushed my changes, including some more, discussed here and following, so, ready for another review. 🙂 |
e99c8de
to
1f1cc30
Compare
Just tweaked the 'react-native' mocking commit. FYI, that commit is cherry-picked (with some changes to the commit message) into my current revision of the RN v0.61 upgrade (#4151). Its appearance in both PRs is reviewable, but when one PR gets merged, we should remove the commit from the other PR branch. 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one substantive thing this needs is a rebase across the Jest mocking changes (as revised and merged via #4151). See also small comments below -- then this will be ready to merge!
src/webview/MessageList.js
Outdated
* - On iOS: We can't easily hardcode this because it includes UUIDs. | ||
* So we bring it over the React Native bridge in ZLPConstants.m. | ||
* - On Android: Different apps' WebViews see different (virtual) root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* - On iOS: We can't easily hardcode this because it includes UUIDs. | |
* So we bring it over the React Native bridge in ZLPConstants.m. | |
* - On Android: Different apps' WebViews see different (virtual) root | |
* - On iOS: We can't easily hardcode this because it includes UUIDs. | |
* So we bring it over the React Native bridge in ZLPConstants.m. | |
* | |
* - On Android: Different apps' WebViews see different (virtual) root |
src/webview/MessageList.js
Outdated
* | ||
* See `check_path_name` in `tools/build-webview` for information on | ||
* what folder this is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is implementation details, not interface, so doesn't belong in jsdoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And thinking a bit more about this, the real value of this cross-reference is about finding what files go into this directory, not finding where it lives in the build tree. (The existing comment isn't very specific about this distinction.) So instead of mentioning check_path_name
, maybe:
* | |
* See `check_path_name` in `tools/build-webview` for information on | |
* what folder this is. | |
* | |
* This is the folder populated at build time by `tools/build-webview`. |
(and jsdoc is a fine place for that.)
1f1cc30
to
0b5fb09
Compare
Thanks for the review! Just pushed my changes. |
0b5fb09
to
7e83bfc
Compare
Just resolved a conflict. |
We're about to expose a constant here that doesn't come from the Info.plist (the "information property list file") [1]. It seems silly to make a new NativeModule for the new constant, so, rename the file so it makes sense for both constants. [1]: https://developer.apple.com/library/archive/documentation/General/Reference/InfoPlistKeyReference/Articles/AboutInformationPropertyListFiles.html
This points to a location that's part of the built app (i.e., something from `[NSBundle mainBundle]`. We tried using a convenient-looking React Native method, `RCTBundleURLProvider.resourceURLForResourceRoot` [1]. This would be useful if we wanted the files in `ios/webview` as they're served by the packager if the packager is running (i.e., at a URL like http://192.168.0.108:8081/ios/webview/index.html). We don't, particularly. In theory, seeing updates to, e.g., `ios/webview/base.css` without having to rebuild might be nice...but things are set up so that we never change that file manually. Instead, we expect it to be generated for us at build time, as a build phase (that runs `tools/build-webview`). Best not to give any incentive to edit an auto-generated file. Also, that React Native method just won't work nicely for us, it seems. I haven't found a way to use it to express that we want "/ios/webview/index.html" when we're talking to the packager, and just "/webview/index.html" when we're talking to `[NSBundle mainBundle]`. And: Mock `ReactNative.NativeModules.ZLPConstants`, as we probably should have done for `appIdentifierPrefix`. But we're about to remove that, in the next commit. See also discussion [2]. [1]: Added in facebook/react-native@150c522be. [2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Resource.20URLs/near/924227
In a recent commit, we exposed this from the native side of the React Native bridge. Use it, make `baseUrl` a URL object, and remove various hacks that are now unnecessary. We also rename `assetsPath` to `webviewAssetsUrl`: there's no question that it's a URL now, and it specifically refers to the `webview/` directory in the platform-specific assets folder. The platform-specific assets folder URL is extracted as `assetsUrl`.
Since it doesn't need to exist, its existence is confusing. As Greg says in discussion [1], it's natural to assume, mistakenly, that it's there for a reason and plays some role. [1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Resource.20URLs/near/927396
From your most recent review, at #4183 (review):
I've done that rebase, and those comments were indeed small (and I believe I've addressed them), so I'll go ahead and merge this. 🙂 |
7e83bfc
to
ae7ad99
Compare
Following discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Resource.20URLs/near/924227.
This should be NFC on Android, but I haven't tested—so I'm marking this as a draft until I do so.Done!I've tried to be faithful to the discussions around the labels we choose, etc., at 28bd508 and at links from there. But I may have missed something.