Extract the typeface-finding logic to findEffectiveTypeface

[jakub-trzebiatowski]

Summary:

Extract the typeface-finding logic to findEffectiveTypeface extension methods.

Changelog:

[INTERNAL] [CHANGED] - Extract the typeface-finding logic to findEffectiveTypeface

Test Plan:

[jakub-trzebiatowski]

This is not a very big deal, but helps me later down the road in my work on #42602

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	17,066,844	+12
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	20,450,434	+2
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: cfc0ba0
Branch: main

[NickGerleman]

I like splitting this out to be shared, but am wondering why this needs to be part of an inheritance hierarchy, if it's effectively just a pure function accepting some inputs?

[jakub-trzebiatowski]

It doesn't need to be; I just implemented it this way.

Suppose you refactor this to be a non-method. In that case, you'll notice that both implementations pass their properties as the arguments, and these properties basically have the same name and (arguably) similar/same semantics.

I personally don't think about it as a "public" inheritance for the sake of this class' users, but more like a mix-in.

Existing programming languages are quite limiting in expressing such concepts, and often, an interface is the best thing you'll get.

In this case, the classes that implement this interface are "relatively" internal, so we've got the consequences under control, I think.

[NickGerleman]

This starts getting into territory of subjectivity, and this being structured as an interface is better than pure inheritance, but it means at the call-site of findEffectiveTypeface, we no longer know what state is actually an input to the algorithm, without looking up the interface et all.

In this case, where the state never changes over time, we've kinda just taken ReactTypefaceUtil, curried a couple params to a new function. Imo the indirection/boilerplate makes the code less easy to quickly reason about, instead of more (esp for ReactEditText).

[jakub-trzebiatowski]

This starts getting into territory of subjectivity,

It sure does

we no longer know what state is actually an input

It's nearly always the case with deduplication that the implementation details are taken away from you on the use site. You just assume that it does what it needs to do.

It can be argued both ways. When things are modeled as Ints or Strings, as it's the case in this project, you could also argue that an explicitly modeled mixin interface brings benefits, as it gives confidence that the right things are passed to the right places.

I was aware of the other way to model this, and I agree that it has some advantages. I decided to go this way.

Let me know if you consider this a blocker.

[jakub-trzebiatowski]

I can add that the main reason I implemented this is that I add a third use later (if this changes anything).

[jakub-trzebiatowski]

Sorry, I misinformed you. I ended up not adding a new implementation. But you can see how this interface is used in my work-in-progress implementation:

(relevant usage)

[jakub-trzebiatowski]

@NickGerleman Did the real usage bring any new light here? Do you think we should "inline" the interface in this place?

[jakub-trzebiatowski]

Also, could we clarify what you meant by...

I like splitting this out to be shared [...]

...? If we nuke the FontAttributeProvider interface, this whole PR is close to being a no-op, so I think we'd need to close it in such a case.

[jakub-trzebiatowski]

@NickGerleman Bump 🙂

[NickGerleman]

Yeah, I still don't love the interface here.

[react-native-bot]

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[react-native-bot]

This PR was closed because it has been stalled for 7 days with no activity.