Skip to content
This repository has been archived by the owner on Dec 5, 2024. It is now read-only.

Simplify flow type distribution #158

Closed
wants to merge 1 commit into from
Closed

Simplify flow type distribution #158

wants to merge 1 commit into from

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented May 1, 2018

In this diff I replaced flow-copy-source with much more simple and less
verbose way to distribute flow types.

I suggest to reuse flow types which already exist in src folder by
reexporting them.

This also requires to publish src.

In this diff I replaced flow-copy-source with much more simple and less
verbose way to distribute flow types.

I suggest to reuse flow types which already exist in `src` folder by
reexporting them.

This also requires to publish `src`.
@FezVrasta
Copy link
Member

What's wrong with the current approach?

@TrySound
Copy link
Contributor Author

TrySound commented May 1, 2018

It requires unnecessary tool and produces a lot of unnecessary files, which can be reused instead of duplication.

@FezVrasta
Copy link
Member

Yes but with the current method we avoid to ship tests and other unused stuff to npm. Also, it's quite common to ship the .flow.js files near the .js files, your approach is way less common.

@TrySound
Copy link
Contributor Author

TrySound commented May 1, 2018

Also forgot to mention. Currently flow are placed in es folder where they are absolutely useless. Developers usually import from react-popper not react-popper/lib/es.

@FezVrasta
Copy link
Member

If you import from react-popper and your bundler supports ES modules it should read them from lib/es, isn't that the case?

@TrySound
Copy link
Contributor Author

TrySound commented May 1, 2018

@FezVrasta .js.flow files are not for bundler. They are consumed by flow. Flow don't care about module field.

@FezVrasta
Copy link
Member

Ok so we can simply change flow-copy-source to copy to cjs instead of es, but I'd keep it.

@TrySound
Copy link
Contributor Author

TrySound commented May 1, 2018

My way to distribute flow types is like bundling, only one file is placed in dist. Also I'm gonna to propose cjs and esm bundles instead of bunch of babeled files.

@FezVrasta
Copy link
Member

I prefer the current approach, really.

About bundles, I don't think there's need to generate them, anyway nobody is going to be able to use them in the browser directly.

@TrySound
Copy link
Contributor Author

TrySound commented May 1, 2018

The problem that currently there are two ways to consume the library. With single entry point (import { Popper } from 'react-popper') and with path imports (import Popper from 'react-popper/lib/es/Popper'). Bundling reduces this choice and keep users code consistent.

Also bundling is a way to optimize user bundler work.

@FezVrasta
Copy link
Member

We can't provide an UMD bundle because some of our dependencies don't ship UMD bundles.

@TrySound
Copy link
Contributor Author

TrySound commented May 1, 2018

UMD usually means bundled dependencies. Only react and react-dom shouldn't bundled in this project.

@FezVrasta
Copy link
Member

In any case, that would be an additional dist target, the ones we have should stay as they are.

@FezVrasta
Copy link
Member

I fixed the flow-copy-source command in #154.

If you'd like to add an UMD build please open a new PR, thanks!

@FezVrasta FezVrasta closed this May 1, 2018
@TrySound TrySound deleted the simple-flow-distribution branch May 25, 2018 07:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants