feat(popper): Improved type definitions

[edcarroll]

Fixes issues introduced by included type definitions conflicting with @types definitions.

Also includes improved modifier definitions, as well as exposing the options property on the Popper class.

[CLAassistant]

All committers have signed the CLA.

[edcarroll]

@giladgray these changes will break the newly created react-popper type definitions, however will fix the builds for everyone originally relying on @types/popper.js

[FezVrasta]

Waiting for @giladgray feedback to merge this.

I'm willing to merge this because the type definitions introduced in 1.11.0 are actually breaking changes to the previous version so it's okay to "break" them to reintroduce stability with the previous ones.

[giladgray]

I'm going to need some time to think about this change. The move to put everything in declare module was deliberate to avoid cluttering the global namespace with symbols from un-imported libraries.

In the short term, please remove @types/popper.js to fix the conflicts. That package should actually be removed from DT now that the library publishes its own typings: https://github.com/DefinitelyTyped/DefinitelyTyped#removing-a-package

[FezVrasta]

The thing is that I see that with your way to define types people have to import them all with something like import * as PopperTypes from "popper.js"; which seem redundant.

I really like to be able to use import Popper from 'popper.js' both for the lib and the types, what's wrong with it?

[edcarroll]

@giladgray I can see where you're coming from but I think the more pressing issue is that the new definition files included with the library break typescript builds relying on the external types, as @FezVrasta mentioned above.

This is something that can only be fixed by using the old structure for now, and if the new structure is an improvement (and I can see where you're coming from, I just need to research it a bit more to be sure of things) then it should be added in the 2.0 release.

[edcarroll]

I've made this gist that I've tested and works.

It is similar to what you were doing @giladgray, however it retains support for import Popper from "popper.js" whilst keeping everything in a single module namespace.

However, popper.js when used as a standalone library adds a Popper class to the global scope, so on balance I'm not sure it's a bad idea to put Popper on the global namespace (which the gist above avoids doing) - as what if you want to use the library with Typescript but don't have a module loader (which apparently still happens!)?

@FezVrasta what are your thoughts on this? I can't see a reason to make Popper an import only library due to the number of people that don't use module loaders.

Please forgive me if I'm completely off the mark here!

[FezVrasta]

I haven't enough knowledge of TypeScript to give meaningful feedbacks, the only reason I'm willing to merge this PR is to fix the API contract with previous versions.

[giladgray]

@edcarroll the idea behind the types here is you should not need @types/popper.js as these are strictly better since they ship with the source NPM package itself. the main benefit here is you can safely expose popper.js types in your library's API without requiring an @types dependency for non-TS consumers.

[giladgray]

also, have you folks tried this import strategy? it should be the new way:

import Popper, { Modifiers, Placement } from "popper.js"
// Popper is a real JS constructor, Modifers & Placement are just TS types

[edcarroll]

@giladgray don't worry I'm not trying to debate whether the types should be included - I'm fully aware of the benefits of including the types here (and am very happy they are being included 😄) - the point I'm trying to make is the currently included typings have broken the builds for everyone using the (now defunct) external ones as you've modified how the types must be imported.

I know you can just uninstall them but the point is (as @FezVrasta has mentioned a few times as well) that right now they're a breaking change and so shouldn't be included in the form you've suggested until version 2 - do you see what I mean?

Regarding the import strategy you've just mentioned, I wasn't aware of it at all and do quite like it!

Btw, do you see what I mean regarding the global namespace and the fact that the popper.js types should be available due to how the library is used by some people (i.e. as a plain script tag).

[FezVrasta]

Could we provide both methods together?

[FezVrasta]

We got this issue #369

If it can be fixed merging this PR I'm willing to merge this as is, then you guys can improve the type definitions however you like, I just care to have the library in a stable situation

[edcarroll]

@FezVrasta my PR should fix this error - I believe it's due to strictNullChecks being enabled on his configuration, which my updated typings account for 😄

[FezVrasta]

@giladgray and @edcarroll, as I said, feel free to send PRs to improve the current definitions. I don't have experience with TypeScript so I can't be of any help unfortunately.

[giladgray]

sounds good, will do when i have some time

[rajington]

@edcarroll about it breaking react-poppper, is that something that should now be updated there?

I could submit a PR that literally just changes this line: https://github.com/souporserious/react-popper/blob/master/react-popper.d.ts#L3:L3 to

import PopperJS from "popper.js";

if you think that's sufficient.

[edcarroll]

@rajington it does need to be updated indeed, as it relies on type definitions that aren't in place anymore. I think the change you've mentioned should be sufficient, the other changes made don't appear to affect the usage in that file. Thanks 😄

[adidahiya]

I have a few bits of feedback here:

1. Making this package incompatible with @types/popper.js is a small breaking change, but it is not a functional or API breaking change. Therefore I believe is OK to introduce types bundled in popper.js which are incompatible with @types/popper.js and not major version bump this package. It should instead be listed as a "manual upgrade step" in a minor release of Popper.
   • Think about the alternative: if every time an NPM package chose to switch to bundling its type definitions (something that is quite useful for libraries that get pulled in as transitive dependencies), then they would need to be bumped to the next major version. This would end up creating a lot of noise across the ecosystem.
2. The change to export default is semantically wrong. I understand that the intention in the popper.js source code is to expose the Popper class as a default export, however you must use these guidelines when writing type definitions.
   • When I run node -p 'require("popper".js'), I get the Popper class.
   • When I run node -p 'require("popper".js').default, I get undefined.
   • If you really want to expose this export as default in both ESM and TS, you must change the way popper.js/dist/umd/popper.js is produced such that module.exports.default is populated in a Node runtime (I'm running Node 8.3)
3. Because of (2), Update to use latest typings from popper.js react-popper#46 should not merge.

[FezVrasta]

I'm really losing hope with all this TypeScript shit...

Please send PRs and discuss with the other interested people about all this problems.
If I see people can't find an agreement I'm going to drop support for TS, because all this is really getting enough...

[rajington]

@FezVrasta I hear you, definitely frustrating to get started with writing typedefs. Hopefully the community that cares about typedefs can handle it without your involvement. If someone wants to be in charge of it, maybe @adidahiya since he seems knowledgeable about the matter, you could create a code owners file for the typedef file only.

In general the direction is trying to move away from DefinitelyTyped, but if it's a big enough problem for you, going back to that is always an option.

And thanks for the library.

[adidahiya]

@FezVrasta hey, sorry about that, my intention wasn't to blame you or cause additional stress -- I do intend to help contribute here and take on some of the maintenance burden. I wanted to post my comment here so that I'd be sure the interested parties would see it. I'll move further discussion to a new PR. Thanks for the library!

[FezVrasta]

no problem guys, I'm just already against TypeScript (I use Flow) and seeing all this churn around a so simple feature makes me hate that damn thing even more 🙄

[MaxDesiatov]

Anyone here having problems with these typings? I have an issue that's stably reproducible, but not sure if this is an issue with popper.js typings or react-popper typings. Steps to reproduce:

1. mkdir test && cd test && yarn add react react-popper
2. echo "import { Arrow, Manager, Popper, Target } from 'react-popper';" > test.ts
3. echo '{ "compilerOptions": { "jsx": "react", "target": "es6", "module": "commonjs", "allowSyntheticDefaultImports": true } }' > tsconfig.json
4. tsc

Compilation always fails with:

node_modules/react-popper/react-popper.d.ts(30,36): error TS2694: Namespace ''popper.js'' has no exported member 'Placement'.
node_modules/react-popper/react-popper.d.ts(36,26): error TS2694: Namespace ''popper.js'' has no exported member 'Modifiers'.
node_modules/react-popper/react-popper.d.ts(37,26): error TS2694: Namespace ''popper.js'' has no exported member 'Placement'.

Any suggestions?