Stop using the js-sdk's compare function #12782
Conversation
The file is supposed to be a js-sdk internal module so we shouldn't have been using it, and now it uses the native collator, it's completely trivial. It was also causing Intl.Collator to be accessed at the module scope which risked it beating the modernizr check.
dbkr
added
the
T-Task
and change the one use of it to just intantiate a collator and use it. This was marked as internal module so this shouldn't be a breaking change. Of course, react-sdk was using it. Requires: matrix-org/matrix-react-sdk#12782
The Modernizr check should be updated to not load any unsafe code, by importing as little as possible, maybe nothing. |
| @@ -17,7 +17,6 @@ limitations under the License. | |||
|
|
|||
| import React from "react"; | |||
| import { Room, RoomEvent, RoomMember, RoomMemberEvent, MatrixEvent } from "matrix-js-sdk/src/matrix"; | |||
| import { compare } from "matrix-js-sdk/src/utils"; | |||
There was a problem hiding this comment.
matrix-js-sdk/src/utils is intended to be reusable though not necessarily stable. Given we use a load of utils from that export why are you picking on this one in particular?
There was a problem hiding this comment.
Since it was instantiated at the module level, it was running before modernizr when I was trying to write the test. I figure rather than rearranging stuff to try to get the execution order right, the best way to fix this was to just not arbitrarily instantiate a static collator and instead make one when we need it, at which point the compare function is adding no value at all.
There was a problem hiding this comment.
But element-web can't assume any of its dependencies don't do that. Sure we can fix that in the js-sdk but not in other dependencies. So instead we should write our code to defensively handle that situation.
There was a problem hiding this comment.
I don't disagree, although I'd need to dig more into exactly what order things get run in to do so. This felt like a thing that was still worth doing anyway.
There was a problem hiding this comment.
Its a case of removing the import here https://github.com/element-hq/element-web/blob/develop/src/vector/url_utils.ts#L17 given it is imported by https://github.com/element-hq/element-web/blob/develop/src/vector/index.ts#L24C40-L24C49
There was a problem hiding this comment.
Although that said, if you strongly think that's a better thing to do than this then I'll stop killing myself trying to get the test coverage on this PR up to the bar.
There was a problem hiding this comment.
I think this is a band-aid for a single cause, which is quite esoteric given that https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Collator#browser_compatibility has been supported for years, so instead making the "bootloader" resilient to any such calls for more recent APIs in dependencies we control and those we do not would be more sane IMO
There was a problem hiding this comment.
Oh well, test didn't involve fixing other app bugs like the other one did, so this may as well live as tidy-up fix anyway. We might want to consider just loading a small stub from the HTML and loading the full app as an async import, which would give us more flexibility to do loading screens and such as well as have complete control over what executes in the stub.
* Remove the compare function from utils and change the one use of it to just intantiate a collator and use it. This was marked as internal module so this shouldn't be a breaking change. Of course, react-sdk was using it. Requires: matrix-org/matrix-react-sdk#12782 * Add simple not-a-perf-test test * recalculate repeatedly otherwise we aren't testing anything different * Use fewer members as it was making the test take a bit too long
The file is supposed to be a js-sdk internal module so we shouldn't have been using it, and now it uses the native collator, it's completely trivial. It was also causing Intl.Collator to be accessed at the module scope which risked it beating the modernizr check.
We just need to instantiate a collator each time, which also potentially allows us to use different collations as appropriate in the future.
Checklist
public/exportedsymbols have accurate TSDoc documentation.