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

Improve WebView build system #2690

Closed
wants to merge 19 commits into from
Closed

Conversation

borisyankov
Copy link
Contributor

@borisyankov borisyankov commented Jun 18, 2018

This is an important rework of how building the WebView assets work.
Considered 'merge-ready', it is the base for further improvements.

This rework now enables the WebView to load its assets (styles, js, potentially fonts) from external files and not need to be built into the single JS file.

This approach is needed so we can embed:

  • fonts
  • emoji or other images

Makes it simpler to embed:

  • polyfills and other vendor JS files
  • styles that are pre-generated (code-highlighting, LaTeX/KaTeX)

In addition, this makes the reported error line-numbers in JS reliable.

This also adds:

  • full code-formatting
  • LaTeX styling

@gnprice
Copy link
Member

gnprice commented Jun 19, 2018

Excellent, thanks @borisyankov ! It'll be great to have this system cleaned up -- I'm already appreciating how it makes the IDE able to highlight and show warnings in theme.css like a normal CSS file. :-)

A few things I'd like to see changed before merging this:

  • There are a lot of spots where the changes in one commit don't really make sense without the context of some later commit.
    • For example: one commit adds emoji.css and explains that the point is to stop generating the data dynamically in cssEmojis.js, but it doesn't actually make any changes related to cssEmojis.js. Then a few commits later we stop using cssEmojis.js in css.js, in a commit that's doing a lot of other stuff and doesn't mention that change in its commit message. In the end it looks like the actual cssEmojis.js file sticks around even though it's not used anymore.
    • An example of what I'd much rather see: one commit (a) adds emoji.css, (b) switches css.js to use it, and (c) deletes cssEmojis.js -- and it doesn't do anything else. I think this would make it a lot easier to follow the changes; it'd probably also make it less likely for something to fall through the cracks like leaving the unused file around, precisely because it's so much easier to look back at and reread.
  • It's kind of unfortunate to make a bunch of copies of all these files. For one thing, it makes them all look like source files you have to edit when making changes -- and it makes them look like they might differ between the platforms, which would be really complex to read.
    • There must be some reasonable way we can script this, making these copies happen at build time. E.g. on the Android side, we can add a few lines in the Gradle scripts to just invoke our build-webview.js. Let's do that instead.
    • I know this was already awkward this way with generatedEs3.js, but it gets a lot worse with so many files, so I'd like to solve the problem properly before doing that.
    • If there is a generated file we end up keeping in the repo, I'd want it to be super clear (a) that it is generated and (b) how to edit/regenerate it. Like the header I added at the top of generatedEs3.js.
  • Small Git tip: don't delete a -diff line from .gitattributes. It remains useful when looking at history from before the file was removed -- e.g. when reading this branch (where that line is gone in HEAD), I get a big diff for where you remove that file.

@borisyankov borisyankov force-pushed the webview-build branch 2 times, most recently from 9043de7 to b362ea1 Compare June 19, 2018 15:13
@zulip zulip deleted a comment from zulipbot Jun 19, 2018
@borisyankov borisyankov force-pushed the webview-build branch 2 times, most recently from 26e27b4 to 8d2e219 Compare June 20, 2018 00:26
@borisyankov
Copy link
Contributor Author

I did undo the removal of the diff line.

codeToCss.js is not deleted because we will use it in future to regenerate the css static file, if the input data changes. I did update the commit message to include that information.

@borisyankov
Copy link
Contributor Author

About the duplication of files. I researched today, and while Android is reasonably straight-forward, not so with iOS.

I will continue exploring a solution for both platforms but suggest we do work on merging this first?

@gnprice
Copy link
Member

gnprice commented Jun 21, 2018

codeToCss.js is not deleted because we will use it in future to regenerate the css static file, if the input data changes. I did update the commit message to include that information.

Ah, OK, thanks.

Would you add something in the repo explaining how to do that regeneration? A little script in tools/ would be ideal; but if for any reason it's a pain to script, then some written instructions would be totally fine. I just want to make sure it's easy for any of us (including future-you) to reproduce what you did.

The rest of that example still applies -- I'd be much happier seeing a commit that added emoji.css, switched css.js to use it, and didn't do anything else.

Similarly, I'd like to have one commit (a) add matches-polyfill.js, (b) switch script.js to use it, and (c) remove matchesPolyfill.js. And another commit that does the same thing for smooth-scroll.js / smoothScroll.min.js.

I think a good commit, or series of commits, to start with would be to just switch from generatedEs3.js to the zulip.js resource. That has the fewest moving parts because that's already a generated file; and once that's done, the others can follow by building on the infrastructure for that one. I would gladly merge some commits that do that, as the first N commits on a branch, even before we're done with any revisions on the other changes -- that would help us move things forward.


About the duplication of files. I researched today, and while Android is reasonably straight-forward, not so with iOS.

I will continue exploring a solution for both platforms but suggest we do work on merging this first?

Sure; I'd be OK with merging for now a version with the duplicate files. We can focus first on the other aspects discussed above.

@borisyankov borisyankov force-pushed the webview-build branch 2 times, most recently from 8a53bf9 to 9945dc0 Compare June 22, 2018 23:12
Add a constant `baseUrl` that is platofrm-dependent and points to
the URL of the local resource directory on the device.

On iOS it is just a relative path `./`.

On Android it points to the asset directory `file:///android_asset/`.
We also add a subfolder `webview` to keep stuff organized.
Function is given a resource file name and returns the URL to be
used by WebView to refer to a resource on that device.
Instead of importing these as string files during build time,
we will refer to the files as resources during runtime.

* add the `Element.matches` polyfill
* add the unminified version of `smooth-scroll`
* 'theme.css' are all the normal styles
* 'dark.css' are the dark-theme styles
Instead of generating the data dynamically (in cssEmojis.js) just
store it in `emoji.css`.

This makes the dynamically generated parts much shorter and
easier to debug.

We keep the `cssEmoji.js` file intact, as we can use it to generate
the `emoji.css` file again if input data changes.
Instead of just compiling the js file, now we do that but also
copy the files in `src/webview/assets/` to be imported from the
webview directly.

Now the compiled JS file is not a string that gets imported but
is a normal JS file.

The command to be run now is `yarn build-webview` - to better
reflect the process.
This is the result of running `yarn build-webview`

The files need to be copied twice for the two platforms.
The process for Android is automatic and nothing special is needed.

For iOS this is done from within XCode:
* open the project
* open Project Navigator
* right click on ZulipMobile
* select 'Add Files to "ZulipMobile"...'
* add the `ios/webview` folder

When adding more assets in the future we'll need to repeat the
process but only for the new files.
Providing a `baseUrl` relative to device's assets location
allows us to load external resources.

Initially we set `baseUrl` to realm's root so relative URLs point
correctly. We did not need that though, as currently all URLs are
either absolute or we transform them to absolute.
Delete the imported-string-from-js code used before and load
css styles from device's local assets.
Instead of loading imported-strings-as-js we just refer to the
asset files of the device.

Also, move the `scripts` code to the bottom of the page.
This was initially moved to the top so the reported error code-lines
are more predictable (the different html was messing up the count).

Now we don't need that, so simplify.
This is the full `pygments.scss` file complied.
Also add the `code.css` file via XCode.
Removes the very small subset of code-highlighting styles from
`css.js` (manually added when we used native-rendering) and use
the full pygments `code.css` file as resource.
Makes dark theme styling consistent with how this is done in the
web app. Instead of conditionally including `dark.css` do include
it always, but make the styles apply only when `body` has a class
of `night-mode`.

This makes the `code.css` file, already having styles in this
format work well in night mode.
This is consistent with how the web app styles the code blocks.

`.codehilite` has a background by default. Overwrite the style
instead of changing the styles in `code.css` keep the file
unchanges (directly coming from 'pygments') for easier update later.
This is the main `katex.less` file compiled to css
https://github.com/Khan/KaTeX/blob/master/src/katex.less

We do not yet include the custom fonts, but still the improvement
in rendering LaTeX is significant.
Remove the `generatedEs3.js` file and add the two asset folders.
Use `yarn build-webview -w` or `yarn build-webview --watch`

The input files are watched for changes. If asset files change,
they are copied again to the asset folders. If the JavaScript file
changes, it is recompiled.
@zulipbot
Copy link
Member

Heads up @borisyankov, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@gnprice
Copy link
Member

gnprice commented Sep 24, 2018

BTW, my current thinking is it'd be good to turn our attention back to this after we complete the RN 0.56 and 0.57 upgrades (#2788 and #2955); we've seen a couple of instances recently of places where it'd be good to have this done.

@gnprice
Copy link
Member

gnprice commented Feb 16, 2019

Bumping the priority because this blocks #2660, which people keep asking for and I just bumped.

rk-for-zulip added a commit to rk-for-zulip/zulip-mobile that referenced this pull request Oct 4, 2019
At present, when loading content into the WebView, we inject all
non-image assets as part of the generated HTML. This is more than a
bit wasteful, considering that large swaths of the supporting CSS and
JavaScript is entirely static from invocation to invocation;
additionally, it makes KaTeX support somewhere between "nontrivial"
and "impossible".

This commit creates a build subsystem which can inject static
webview-supporting assets into the application bundle, into a
`webview` directory.

We also create a stub directory for the subsystem copy assets from; at
present this contains only a README.md file (which will *not* be
copied), but this will be expanded in the following series of commits.

Inspired by zulip#2690. Assists with zulip#2660, zulip#3595, and potentially many
other issues.
rk-for-zulip added a commit to rk-for-zulip/zulip-mobile that referenced this pull request Oct 4, 2019
At present, when loading content into the WebView, we inject all
non-image assets as part of the generated HTML. This is more than a
bit wasteful, considering that large swaths of the supporting CSS and
JavaScript is entirely static from invocation to invocation;
additionally, it makes KaTeX support somewhere between "nontrivial"
and "impossible".

This commit creates a build subsystem which can inject static
webview-supporting assets into the application bundle, into a
`webview` directory.

We also create a stub directory for the subsystem to copy assets from;
at present this contains only a README.md file (which will *not* be
copied), but this will be expanded in the following series of commits.

Inspired by zulip#2690. Assists with zulip#2660, zulip#3595, and potentially many
other issues.
rk-for-zulip added a commit to rk-for-zulip/zulip-mobile that referenced this pull request Oct 9, 2019
At present, when loading content into the WebView, we inject all
non-image assets as part of the generated HTML. This is more than a
bit wasteful, considering that large swaths of the supporting CSS and
JavaScript is entirely static from invocation to invocation;
additionally, it makes KaTeX support somewhere between "nontrivial"
and "impossible".

This commit creates a build subsystem which can inject static
webview-supporting assets into the application bundle, into a
`webview` directory.

We also create a stub directory for the subsystem to copy assets from;
at present this contains only a README.md file (which will *not* be
copied), but this will be expanded in the following series of commits.

Inspired by zulip#2690. Assists with zulip#2660, zulip#3595, and potentially many
other issues.
rk-for-zulip added a commit to rk-for-zulip/zulip-mobile that referenced this pull request Oct 10, 2019
At present, when loading content into the WebView, we inject all
non-image assets as part of the generated HTML. This is more than a
bit wasteful, considering that large swaths of the supporting CSS and
JavaScript is entirely static from invocation to invocation;
additionally, it makes KaTeX support somewhere between "nontrivial"
and "impossible".

This commit creates a build subsystem which can inject static
webview-supporting assets into the application bundle, into a
`webview` directory.

We also create a stub directory for the subsystem to copy assets from;
at present this contains only a README.md file (which will *not* be
copied), but this will be expanded in the following series of commits.

Inspired by zulip#2690. Assists with zulip#2660, zulip#3595, and potentially many
other issues.
gnprice pushed a commit that referenced this pull request Oct 11, 2019
At present, when loading content into the WebView, we inject all
non-image assets as part of the generated HTML. This is more than a
bit wasteful, considering that large swaths of the supporting CSS and
JavaScript is entirely static from invocation to invocation;
additionally, it makes KaTeX support somewhere between "nontrivial"
and "impossible".

This commit creates a build subsystem which can inject static
webview-supporting assets into the application bundle, into a
`webview` directory.

We also create a stub directory for the subsystem to copy assets from;
at present this contains only a README.md file (which will *not* be
copied), but this will be expanded in the following series of commits.

Inspired by #2690. Assists with #2660, #3595, and potentially many
other issues.
@gnprice
Copy link
Member

gnprice commented Oct 31, 2019

Much of what this PR aimed to do has now been accomplished with #3636.

There are still some further things to do. I think those will be best tracked as we see things we need (e.g. to complete #2660) rather than based on this prototype PR, so closing it.

@gnprice gnprice closed this Oct 31, 2019
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.

3 participants