Refactor RTL plugin loading code #3418
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3418 +/- ##
==========================================
+ Coverage 75.50% 75.52% +0.02%
==========================================
Files 241 242 +1
Lines 19230 19210 -20
Branches 4271 4259 -12
==========================================
- Hits 14520 14509 -11
+ Misses 4710 4701 -9 ☔ View full report in Codecov by Sentry. |
|
Man, these render tests are a pain... :-S |
| dispatcher: Dispatcher = new Dispatcher(getGlobalWorkerPool(), 'rtl-text-plugin-dispacher'); | ||
| queue: PluginState[] = []; | ||
|
|
||
| async _sendPluginStateToWorker() { |
There was a problem hiding this comment.
Should this be private?
There was a problem hiding this comment.
I'm not sure if I should start adding private... while I generally agree that this is the right approach, it should probably be a wider usage of these modifiers, so I'm not sure when will be a good time to introduce this...
There was a problem hiding this comment.
Well, the way I look at it is anything that isn't private is technically adding to the API, even if it has an underscore.
There was a problem hiding this comment.
If it makes sense, especially since we're gearing up for 4.x, I'd be happy to go through map.ts and style.ts to mark stuff as private. Maybe some other areas too.
There was a problem hiding this comment.
I agree, but there are plugins that uses these underscore methods and the code itself which uses those, so this is more of a convention than an actual way to block usage... IDK...
Also see here:
There was a problem hiding this comment.
Was writing while you were, so my message is answering you first and not second.
I would advise to get some feedback from the community around this before doing so - changing stuff to private, using our slack channel.
While I generally agree this is the right solution, I'm not sure what it would solve on one hand, but I know it will cause some pain on the other - so the value here is low I believe.
There was a problem hiding this comment.
OK, if you don't think it's worth the pain, I'll stop harping, haha
| } | ||
| } | ||
|
|
||
| async _downloadRTLTextPlugin() { |
| @@ -320,6 +288,18 @@ export class Style extends Evented { | |||
| }); | |||
| } | |||
|
|
|||
| _rtlTextPluginStateChange = () => { | |||
|
This is soooo much better. Thank you for doing this! |
|
Yup, it's a lot clearer now which part runs where and what it actually does. Took me a few hours to reverse engineer this... :-/ |
|
@neodescis can we merge this and decide later on the modifier stuff? |
Certainly! |
Launch Checklist
This is a refactoring of the RTL plugin loading code.
It splits the code into what is done in the worker vs what is done in the main thread (which was a complicated single class).
Moves all the logic into the class, using extending
eventedin the main thread no notify when the plugin is loaded or the state changes.This also removes all the callback hell that was in this class which is not a lot more readable.
It changes the
setRTLTextPluginpublic API and removed the callback from it.CHANGELOG.mdunder the## mainsection.