search

keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
377 stars 104 forks source link

[Web, LMLayer] Utilize Unicode String-handling library #2492

Closed jahorton closed 10 months ago

jahorton commented 4 years ago

There is considerable risk in assuming that all the functions in UTFString work identically to those in kmwstring. kmwstring is a known quantity; UTFString is probably fine but is unknown to us at this time.

We agreed in our review meeting to use kmwstring here, for now, and look at moving to say UTFString across all of KMW and LMLayer in 14.0, once we have had time to validate consistency between the two libraries.

Originally posted by @mcdurdin in https://github.com/keymanapp/keyman/pull/2477

The notes above are in reference to this comment made on the same PR:

In regard to kmwstring.ts: https://github.com/camertron/utfstring (NPM)

Finally found a npm package that accomplishes the same general goals. (Basically, to figure out the right search terms.) The API's a bit different, but its npm installs can easily be compiled in for in-browser use. There'd be a fair bit of tedious work to change KMW over... but it would allow us to not bother making our own separate package for the same thing across our platforms.

Is this something we'd be interested in doing for KMW as a whole, eventually? If so, I'd rather go ahead and use that now for the LMLayer.

We stuck with kmwstring.ts for 13.0 since it's a known factor, but it'd be worth investigating a transition for future versions.

mcdurdin commented 4 years ago

My feeling on this now hasn't really changed. I think there needs to be a demonstrated benefit over and above refactoring and nicer code. Is there a performance benefit? Is the code significantly smaller for distribution?

The cost of introducing 3rd party dependencies, especially for deployed code, is very significant and we don't always choose well.

mcdurdin commented 10 months ago

We decided to not go ahead with this.

jahorton commented 3 months ago

I know we put "not planned" here, but it may be worth noting that our current approach causes "side-effects" when we treat KeymanWeb as a node package / import, which isn't ideal. It may be worth revisiting on the basis of cleaning up the pattern and avoiding side-effects if and when we consider distributing KMW via NPM.

In particular, our addition of instance methods to String.prototype and of a static method onto String are "side effects" from the Node package perspective.

mcdurdin commented 3 months ago

In particular, our addition of instance methods to String.prototype and of a static method onto String are "side effects" from the Node package perspective.

Yes we should refactor that. NPM publishing may be a good time to consider this -- whether we keep using kmwstring refactored away from monkeypatching or utfstring or something else.

We are in a much better place to make that assessment than we were back in v13