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

Load RTL Text plugin on demand #8825

Closed
asheemmamoowala opened this issue Sep 30, 2019 · 5 comments
Closed

Load RTL Text plugin on demand #8825

asheemmamoowala opened this issue Sep 30, 2019 · 5 comments

Comments

@asheemmamoowala
Copy link
Contributor

Motivation

The RTL text plugin is quite large - 37kb (web assembly + js loader) and including it by default on a map increases the load time of the map, even when no RTL text is present.

#6136 proposes emitting a console warning when RTL text is found on the map, but the plugin is not loaded. It would be better to go a step further and make it possible to load the plugin on-demand when such text is first encountered.

Design Alternatives

There are two ways this could be done ,

  1. Emit an rtlpluginmissing event the first time RTL text is encountered and the plugin is not loaded or loading.
  2. Expose a Map option that can be passed into the container with the URL to the RTL Text plugin. The plugin would be automatically loaded by the map the first time RTL text is encountered.
  3. Support right-to-left scripts without requiring plugin (original ticket)

Design

Option 1 doesn't go far enough to simplify the API. All developers hooking up this event will likely be writing the same or similar code. It would be an incomplete solution the problem at hand.
Given the current size of the plugin, and the desire to decrease load times, Option 3 doesn't seem like it would address this need in the short term. Maybe if text placement and shaping was not done in JS, but took advantage of Browser APIs (Canvas or OffscreenCanvas).

Option 2 provide a complete solution for developers interested in reducing initial map load time, and having RTL text show up correctly. Using a URL parameter allows providing a mapbox-hosted or self-hosted URL.

Mock-Up

// Mapbox hosted plugin
var map = new Map ({... , rtlTextPlugin: "https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-rtl-text/v0.2.3/mapbox-gl-rtl-text.js"});

// OR self-hosted plugin
var map = new Map ({... , rtlTextPlugin: "https://my.dev.server/mapbox-gl-rtl-text.js"});

Implementation

At present, there is not a way within GL-JS to detect if a string is arabic or hebrew, and requires r-t-l shaping and styling.

If there is a known range of unicode characters that are supported for RTL, then Workers could message the main thread when such text is encountered. The main thread would load the plugin and re-load the source that triggered the issue.

cc @chloekraw @arindam1993 @kbauhausness

@jingsam
Copy link
Contributor

jingsam commented Oct 9, 2019

Would be better if just reference like this:

<script src='https://api.tiles.mapbox.com/mapbox-gl-js/v1.4.0/mapbox-gl.js'></script>
<script src="https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-rtl-text/v0.2.3/mapbox-gl-rtl-text.js"</script>

or

import { Map } from "mapbox-gl"
import rtlPlugin from "mapbox-gl-rtl-text"

rtlPlugin.patch(Map)

@asheemmamoowala
Copy link
Contributor Author

asheemmamoowala commented Oct 9, 2019

<script src='https://api.tiles.mapbox.com/mapbox-gl-js/v1.4.0/mapbox-gl.js'></script>
<script src="https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-rtl-text/v0.2.3/mapbox-gl-rtl-text.js"</script>

@jingsam In this method, it looks to me like it would always load and parse the RTL text plugin . Is there an on-demand aspect to it as well? Or are you suggesting that it always be loaded?

@jingsam
Copy link
Contributor

jingsam commented Oct 10, 2019

I thought it would be better follow leaflet plugin conventions. when you need it, just explicitly reference it.

@asheemmamoowala
Copy link
Contributor Author

I thought it would be better follow leaflet plugin conventions. when you need it, just explicitly reference it.

Thank for the clarification @jingsam. I wrote up this ticket to allow developers to delay the loading of the plugin until absolutely necessary, instead of having to take the hit on every page/map load.

@arindam1993
Copy link
Contributor

Closing since #8865 is now in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants