Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix .js.flow definitions #829

Merged
merged 2 commits into from Oct 16, 2019
Merged

Fix .js.flow definitions #829

merged 2 commits into from Oct 16, 2019

Conversation

pascalduez
Copy link
Contributor

Hi,

in the current state of affairs, the umd/popper.js.flow definitions shipped alongside the umd/popper.js (package main entry) just doesn't work. It all resolves to any.

That's why you have to do this sort of thing which should not be necessary.

Causes:

  1. There's a typo in the file name :) (this PR fixes it)
  2. .js.flow declaration files have a different behaviour and syntax than "library definitions".
  • They need a // @flow pragma
  • They can't use the declare modules wrapper
  • Types needs to be exported export type ...

So I managed to make this file work locally by applying all the points above, but I'm not sure how would you like to integrate it in the build workflow here. Because it makes this file significantly different from the index.js.flow at the package root. So waiting for feedback before pushing something.

My advice would be, don't maintain two files. Either only include the popper.js.flow (modified file).
Or move the library definition to flow-typed.

@FezVrasta
Copy link
Member

The one in umd should be a direct copy of the one in the root, if you edit /packages/popper/src/index.js.flow you are going to update the one in the UMD folder.

@pascalduez
Copy link
Contributor Author

@FezVrasta Yes, but it would be a "breaking" I guess, after changes this file will not be usable as "library definition" anymore. But probably not much users try to use it as such?

@FezVrasta
Copy link
Member

It was never intended to be used as library definition, I just wanted it to work as is out of the box, so I'd say it falls in the "breaking change of something that never intended to work that way", which is good to me if you ask me.

- `.js.flow` files shipped alongside the main entry point are
different from library definitions, e.g.: flow-typed.
- They need an `@flow` pragma
- They can't be wrapped in a `declare module` block
- They need to export their public types
@pascalduez
Copy link
Contributor Author

@FezVrasta I pushed the updated file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants