Use absolute URL for webview `baseUrl` on iOS. #4183

chrisbobbe · 2020-07-07T01:55:42Z

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.

chrisbobbe · 2020-07-07T02:04:10Z

src/webview/MessageList.js

+          //
+          // [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,


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.

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.

gnprice

Thanks @chrisbobbe ! Looks good apart from the comment above and a few comments below.

src/webview/MessageList.js

gnprice · 2020-07-07T18:24:44Z

src/webview/MessageList.js

@@ -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 }}


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.

it seems to have passed in CI

Ah, I think CI isn't a thing anymore, hence #4174?

Ah indeed 🙂 (and fixed now.)

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 🤷

chrisbobbe · 2020-07-07T23:41:58Z

Just pushed my changes, including some more, discussed here and following, so, ready for another review. 🙂

chrisbobbe · 2020-07-08T23:49:01Z

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. 🙂

gnprice

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!

gnprice · 2020-07-13T21:40:44Z

src/webview/MessageList.js

+ * - 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


Suggested change

* - 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

gnprice · 2020-07-13T21:41:50Z

src/webview/MessageList.js

+ *
+ * See `check_path_name` in `tools/build-webview` for information on
+ * what folder this is.


This is implementation details, not interface, so doesn't belong in jsdoc.

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:

Suggested change

*

* 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.)

chrisbobbe · 2020-07-14T00:14:40Z

Thanks for the review! Just pushed my changes.

chrisbobbe · 2020-07-15T18:59:13Z

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

We don't need this anymore. It was added in 4828ecf for a "Sign in with Apple" strategy that we abandoned in favor of f900521 and following.

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

chrisbobbe · 2020-07-24T23:37:16Z

From your most recent review, at #4183 (review):

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!

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. 🙂

chrisbobbe added a-iOS a-message list labels Jul 7, 2020

chrisbobbe requested a review from gnprice July 7, 2020 01:55

chrisbobbe commented Jul 7, 2020

View reviewed changes

chrisbobbe marked this pull request as ready for review July 7, 2020 17:14

gnprice reviewed Jul 7, 2020

View reviewed changes

chrisbobbe force-pushed the pr-resource-url branch from 914c02e to e99c8de Compare July 7, 2020 23:36

chrisbobbe mentioned this pull request Jul 8, 2020

RN v61 upgrade #4151

Merged

chrisbobbe force-pushed the pr-resource-url branch from e99c8de to 1f1cc30 Compare July 8, 2020 23:47

gnprice reviewed Jul 13, 2020

View reviewed changes

chrisbobbe force-pushed the pr-resource-url branch from 1f1cc30 to 0b5fb09 Compare July 14, 2020 00:09

chrisbobbe mentioned this pull request Jul 15, 2020

Allow viewing message edit history #4173

Open

chrisbobbe force-pushed the pr-resource-url branch from 0b5fb09 to 7e83bfc Compare July 15, 2020 18:59

chrisbobbe added 5 commits July 24, 2020 16:27

ZLPConstants (ios): Remove appIdentifierPrefix.

eb37e6c

We don't need this anymore. It was added in 4828ecf for a "Sign in with Apple" strategy that we abandoned in favor of f900521 and following.

chrisbobbe force-pushed the pr-resource-url branch from 7e83bfc to ae7ad99 Compare July 24, 2020 23:37

chrisbobbe merged commit ae7ad99 into zulip:master Jul 24, 2020

chrisbobbe deleted the pr-resource-url branch November 6, 2020 03:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use absolute URL for webview `baseUrl` on iOS. #4183

Use absolute URL for webview `baseUrl` on iOS. #4183

chrisbobbe commented Jul 7, 2020 •

edited

chrisbobbe Jul 7, 2020 •

edited

gnprice Jul 7, 2020

gnprice left a comment

gnprice Jul 7, 2020

chrisbobbe Jul 7, 2020

gnprice Jul 13, 2020

gnprice Jul 13, 2020

chrisbobbe commented Jul 7, 2020

chrisbobbe commented Jul 8, 2020

gnprice left a comment •

edited

gnprice Jul 13, 2020

gnprice Jul 13, 2020

gnprice Jul 13, 2020

chrisbobbe commented Jul 14, 2020

chrisbobbe commented Jul 15, 2020 •

edited

chrisbobbe commented Jul 24, 2020 •

edited

Use absolute URL for webview baseUrl on iOS. #4183

Use absolute URL for webview baseUrl on iOS. #4183

Conversation

chrisbobbe commented Jul 7, 2020 • edited

chrisbobbe Jul 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jul 7, 2020

chrisbobbe commented Jul 8, 2020

gnprice left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jul 14, 2020

chrisbobbe commented Jul 15, 2020 • edited

chrisbobbe commented Jul 24, 2020 • edited

Use absolute URL for webview `baseUrl` on iOS. #4183

Use absolute URL for webview `baseUrl` on iOS. #4183

chrisbobbe commented Jul 7, 2020 •

edited

chrisbobbe Jul 7, 2020 •

edited

gnprice left a comment •

edited

chrisbobbe commented Jul 15, 2020 •

edited

chrisbobbe commented Jul 24, 2020 •

edited