fix: TypeScript types broken with latest popperjs-core

[bengry]

Fixes #351

[FezVrasta]

Thanks for the PR! I have not a lot of experience with TS so I'll wait for @atomiks's go ahead before I proceed to merge it.

[atomiks]

Works well with the change. I'd like to have this as a default in core too (typed built-in modifiers, untyped custom) but I don't know how the Flow conversion would handle this.

[FezVrasta]

Works well with the change. I'd like to have this as a default in core too (typed built-in modifiers, untyped custom) but I don't know how the Flow conversion would handle this.

I'm preparing a PR to update the Flow types of react-popper, and it seems to work just like TS.

[atomiks]

My suggestions were because I wasn't inlining the modifiers array lol 🤦 — your change is fine.

How would I type this with the modifiers array assigned to a variable instead of inlined?

[bengry]

My suggestions were because I wasn't inlining the modifiers array lol 🤦 — your change is fine.

How would I type this with the modifiers array assigned to a variable instead of inlined?

const modifiers: Modifier<'offset' | 'arrow' | 'externalModifier'> = [
  { name: 'offset', ...},
  { name: 'arrow', ...},
  { name: 'externalModifier', ... }
];

Alternatively, I suggest changing:

type StrictModifier<Name extends StrictModifierNames> = UnionWhere<PopperJS.StrictModifiers, { name?: Name }>;
type StrictModifier<Name extends StrictModifierNames = StrictModifierNames> = UnionWhere<PopperJS.StrictModifiers, { name?: Name }>;

So now you don't have to specify the modifiers in the type up-front, if you only need strict ones:

const modifiers: StrictModifier[] = [
  { name: 'offset', ...},
  { name: 'arrow', ...},
];

Since I think most users only use the strict modifiers anyway, this should be fine IMO.

Of course, you'd need to either export the relevant types, or get them through a generic PropsOf<T> type somehow (example for such a type in emotion).

Let me know if you'd like me to make these changes to the PR before you merge it.

[atomiks]

Is it possible to type custom modifiers with this? I'd say it's an important feature

[bengry]

Is it possible to type custom modifiers with this? I'd say it's an important feature

Sure you can:

const modifiers: (StrictModifier | Modifier<'my-modifier'>)[] = [
  {
    name: 'offset',
    options: {},
  },
  {
    name: 'my-modifier',
    options: {
      someProperty: 'foo',
    },
  },
];

Unfortunately you can't omit the my-modifier in line 1 here (and use string instead of example) since then TS will collapse the two types to a union of all modifiers, and you lose type-safety for strict modifiers - so this will compile, even though it shouldn't (when used inside of an array typed as (StrictModifier | Modifier<string>)[]):

{
  name: 'offset'
  options: {
    adaptive: true,
  },
}

I usually prefer inline types for this reason, TypeScript is much more powerful when you let it infer types than when you explicitly tell the compiler what you want.

Maybe there's a workaround here that I'm missing (apart from creating two arrays - const strictModifiers: StrictModifier[] = [ ... ] and const otherModifiers: Modifier<string>[] = [ ... ] and then merging them later: [...strictModifiers, ...otherModifiers]).

[atomiks]

I mean type custom modifers' options like the strict ones (inlined or not)

[bengry]

I mean type custom modifiers' options like the strict ones (inlined or not)

You can type them like that, they should be able to pass just fine to the modifiers prop/option. Are you having an issue with such a case?

Usually MyModifier would come from an external library I suppose?

[atomiks]

Seems good now ✅

[bengry]

@atomiks great, thanks! :)
@FezVrasta can we get this merged and released soon-ish please?

[l0gicgate]

When will this be merged?

[evadecker]

Hi! Thanks for fixing and merging this, was just about to report the issue. Do you know when this fix will be released on NPM?