Convert extend util to TypeScript

[dsevillamartin]

Changes proposed in this pull request:

• Use .ts extension when referring to extend util in core code
• Allow extensions to use .ts in import path through compat for typings to work properly
• Convert extend util to TypeScript

Reviewers should focus on:

• Do we want to create a bunch more exported types/interfaces to make reading the code easier
• I don't know how to make it so the typings accept a nonexistent key for extend
• Are we fine with the new RegExp for the compat proxy to accept .js, .ts and .tsx

Screenshot

Confirmed

• Frontend changes: tested on a local Flarum installation.

[davwheat]

Out of curiosity, what's the reasoning behind stating the file extension? Is it just to fix typings so that they work properly?

[dsevillamartin]

@davwheat Yes. We currently have an extend.js and an extend/index.js, and so no file extension resolves to extend/index.js.

[dsevillamartin]

Currently fixing issues with my proxifyCompat changes, as they break imports of e.g. tags, and others.

EDIT: ^ Fixed

[dsevillamartin]

what happened here

[davwheat]

Rebased onto master

[davwheat]

Do we want to create a bunch more exported types

I'd say that for something like this (utility types), we should declare them in the @types folder we now have. That lets us use them across core without imports, and extensions can use them too (from 1.0.5 now we have the fix for global typings).

[askvortsov1]

LGTM, let's run prettier and revert the regex change, then good to go!

[SychO9]

(from 1.0.5 now we have the fix for global typings).

1.1 actually, since it was merged into master and not 1.0.5 unless you want to cherry pick it.

I am not well informed to know if it's something that needs to be in 1.0.5 and not wait 1.1

[davwheat]

Oops. I'd say we should stick it in 1.0.5 if we can. It's just a developer improvement and waiting a whole release cycle to provide it seems pointless.

[askvortsov1]

Oops. I'd say we should stick it in 1.0.5 if we can. It's just a developer improvement and waiting a whole release cycle to provide it seems pointless.

I would prefer to keep patch releases focused on bugfixes tbh.

[davwheat]

I'm going to dismiss my review as I've contributed a fair bit to this PR.

[askvortsov1]

I'm going to dismiss my review as I've contributed a fair bit to this PR.

I don't believe that's necessary. That being said, @datitisev could you confirm that you approve of the changes?

[dsevillamartin]

@askvortsov1 I... think? I mean now I understand the KeyOfType types even less but I didn't understand them before so that doesn't matter. The issue of using override with non-declared properties still exists, not sure if we wanted to fix that or not. Doesn't really matter as we allow building with TS errors.

[askvortsov1]

The issue of using override with non-declared properties still exists

In those cases, wouldn't we want to modify the prototype directly?

[dsevillamartin]

@askvortsov1 Not if it has the potential to exist but its optional? Cause then you can take advantage of the override... I think my main example was extend(options, 'config') or something in app.request.

[askvortsov1]

@askvortsov1 Not if it has the potential to exist but its optional? Cause then you can take advantage of the override... I think my main example was extend(options, 'config') or something in app.request.

Wdym by "take advantage of the override"? Are there any advantages compared to directly editing the prototype?

[dsevillamartin]

@askvortsov1 Oh I'm stupid, I'm thinking about extend() and not override()

EDIT: wait no i did say extend, i think i need sleep... what is going on
EDIT 2: oh i mentioned both 🤦

In app.request

    extend(options, 'config', (result, xhr) => xhr.setRequestHeader('X-CSRF-Token', this.session.csrfToken));

Here it would be useful because we account for it not existing. My apologies.

[davwheat]

I think if we allow | string, we still benefit from the Intellisense of the KeysOfType<> type? Don't quote me on that tho...

[dsevillamartin]

Maybe we should add a separate util for that then? A guaruntee that the extended/overriden method exists is pretty useful generally.

This'd be breaking then tho. And I honestly don't see a problem with the current implementation 🤷‍♂️

[askvortsov1]

Maybe we should add a separate util for that then? A guaruntee that the extended/overriden method exists is pretty useful generally.

This'd be breaking then tho. And I honestly don't see a problem with the current implementation 🤷‍♂️

Not really, as we'd be changing the type interface, not the implementation.

Its not that there's a problem with the current non-restrictive approach as much as we're losing out on the ability to enforce something statically through the type system: in particular, because defining a function as a composition of other functions isn't quite the same as extending an existing function.

[davwheat]

I think this is fine as-is. The highlighted issue, if anything, will likely help people as they may realise they're extending the wrong thing (if it doesn't exist), rather than failing silently.

If needed, you can use // @ts-expect-error to override TS until we have another way to do it.