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

Add support for flow:main in package.json #6504

Closed
wants to merge 4 commits into from
Closed

Add support for flow:main in package.json #6504

wants to merge 4 commits into from

Conversation

steelbrain
Copy link
Contributor

@steelbrain steelbrain commented Jun 21, 2018

This PR is a resurrection of #1459

Situation

Flow syntax is not natively supported by JS Engines so flow users use tools like babel to strip types from flow to ship it in npm. This means that people that import those packages into their projects don't have typing support and have to in turn again use flow-typed to add support for packages that were originally written in flow.

There are of course workarounds for this, the most popular one being to mirror sources of files in the same folder as dist as [file].js.flow. It has it's own sets of quirks and limitations.

As things are, it's nearly impossible to use Flowtype in a monorepo setup without having to rely on third party tooling.

Why current alternatives aren't enough

The flow community has had this problem for a while and as such demand has given birth to a supply of "fixes" for this including but not limited to

All of them are unreliable and easily break as they all use .js.flow way of working this issue around

Proposed solution

The path this PR tries to take is to add a new field to package.json called flow:main. When available, it'll be used instead of main. It's resolution and handling is no different than that of a main field.

This change is non API breaking and no existing packages running flow would break with this. Furthermore those packages that already are using lib/index.js.flow or similar ways would also be unaffected by this. This change would only affect the packages that explicitly specify the new key.

This PR enables package authors to allow package authors to specify main to lib/index.js (example value) and flow:main to src/index.js (example value). This change when combined with un-ignoring src directory in .npmignore would give flow typings for modules out of the box.

Furthermore, with this change, no special changes have to be done to make typings in mono repo modules work. They should work OOB.

Why flow:main

It's just a proposed solution. Typescript uses types/typings. We could do something similar. I'm open to suggestion for names but I think flow:main is simple, straight forward and gets the job done :)

@rgbkrk
Copy link
Contributor

rgbkrk commented Jun 22, 2018

This is so wonderful, thank you for going after it.

As things are, it's nearly impossible to use Flowtype in a monorepo setup without having to rely on third party tooling.

Agreed. name_mappers for jest, webpack, and flowtype all just to make sure things resolve cleanly and we get the real benefits of flow's typing.

@steelbrain
Copy link
Contributor Author

Glad you agree @rgbkrk

Note to reviewers: CI failure of lsp\diagnostics in AppVeyor is unrelated to this PR and is present in the master branch as well

@Gozala
Copy link
Contributor

Gozala commented Jun 22, 2018

@steelbrain thanks for doing this! It definitely addresses many issues.
I do want to point out that this will be unable to support non-main module imports say “mylib/optional-module” which things like flow-copy-source does.

Would you consider “flow:src”: “./src/“ or something along those lines in addition ? Idea is to tell flow to resolve modules in the package not from it’s root but from a specific path.

@steelbrain
Copy link
Contributor Author

steelbrain commented Jun 22, 2018

@Gozala The issue you have stated is very valid. I would like to point out that resolutions such as the one you recommended have been tried before by other movements, most notably the browser field one https://github.com/defunctzombie/node-browser-resolve

It ended up being so complicated that there's only one package on npm that supports it and hasn't been updated in forever because of how complex it's become. Further I'd like to point out that this limitation is also shared by Typescript with their typings and types manifest fields so it's a pain the JS community would have to tackle together.

For the time being, I think this fix should make lives of people working with flow in monorepo and other setups easier while maintaining simplicity and obtaining parity with Typescript. You can work the stated issue around with the non-ideal .js.flow way.

@rgbkrk
Copy link
Contributor

rgbkrk commented Jun 22, 2018

I do want to point out that this will be unable to support non-main module imports say “mylib/optional-module” which things like flow-copy-source does.

Within our monorepo we used to have import paths like this then realized that it made our module name mappers excessively complex. In fact, we kept running into issues because of how ./lib was where we were resolving.

@Hypnosphi
Copy link
Contributor

Hypnosphi commented Jun 22, 2018

The problem with sharing flow types directly in published library is that Flow is still 0.x, and breaking changes happen quite often. So, most likely the published types will only work when app's flow version matches the one with which the library was written

So I would say that until Flow becomes stable, flow-typed should be the recommended way to share library packages

@steelbrain
Copy link
Contributor Author

steelbrain commented Jun 22, 2018

@Hypnosphi We cannot publish the sources of our private mono-repo modules just to get support for flow typings among them.

The "sharing" part is entirely optional, if someone is afraid their typings would break, they can of course choose not to publish them and still use flow-typed. This PR does not limit that choice by any means, just makes another way possible

@Hypnosphi
Copy link
Contributor

I'm sure it's a valuable feature, I just doubt that it fixes #1996

@steelbrain
Copy link
Contributor Author

steelbrain commented Jun 22, 2018

I would disagree with you there, It does fix it. It does add a way, just like Typescript for packages to publish their types.

Whether flow has matured enough itself to not break compat is a different question. I don't see why this particular option would have to be changed when flow hits 1.x. The types? Yes. The exposing interface? Shouldn't need changes.

@Hypnosphi
Copy link
Contributor

The exposing interface? Shouldn't need changes.

If it was true, there wouldn't be version ranges in definitions from flow-typed

Maybe flow:main could support object as value?

"flow:main": {
  "flow_v0.25.x-": "src/index.js"
}

@steelbrain
Copy link
Contributor Author

steelbrain commented Jun 23, 2018

@Hypnosphi If you disagree with the fix that I've put up, please feel free to open a new PR that does it better and I'd be more than glad to close this one in favor of it :)

There are currently npm modules that are published that already have types with .js.flow, and no versioning for it because you cannot version file names based on flow version. The issue you mentioned is not new to the ecosystem and I believe it's out of scope of this PR to tackle it.

Again I'd like to reiterate that nobody has to specify this new manifest while publishing to npm, it's like the typing sugar. You can keep things as they are in your npm module or you can specify if you want to. It's just like adding an index.js.flow except nicer and doesn't need third party tooling

@satya164
Copy link

Agree with @Hypnosphi. Different flow versions easily can break non-trivial type definitions from my experience. Ability to specify this in package.json is awesome, but it should be a bit more flexible to allow definitions for different flow versions.

Also, @Gozala raises a good point: #6504 (comment), libraries like lodash follow this pattern, so will be useful to address them too.

Even if it wasn't implemented in this PR, maybe the property value could be an object instead of string to support extending t in future?

@saadq
Copy link

saadq commented Jun 28, 2018

+1 on having multi-flow versions being done in a different PR, and having this one merged in as-is. @steelbrain is right in that this PR is consistent with the current behavior with including flow types in your module – you currently cannot specify any flow versioning for your published types. Also he makes a good point that using this property in your package.json is completely optional, you can still do it the old way.

@jozanza
Copy link

jozanza commented Jun 28, 2018

I'm also in agreement with @steelbrain and @saadq. This PR is immediately useful for those of us who make use of flow-typed in their projects for defining libdefs. Plus, the flow:main property is optional for those who don't wish to make use of it.

Versioning is def an issue, but it's one you'd still have if you take the *.js.flow approach or even if you used TypeScript instead. Rather than quickly settle on a convention here, it would be best to address it in a separate issue/PR imho.

@sibelius
Copy link

@mrkev can u comment on this?

how Facebook handle external packages with flow?

@mrkev
Copy link
Contributor

mrkev commented Jul 5, 2018

@sibelius I think we just use library definitions.

This looks like a good idea tbh, but I'm worried about versioning. I want to look at the discussion from the previous one. Unfortunately don't have much time to look deeply into this now, sorry. I'll keep it in my backlog.

@ardok
Copy link

ardok commented Jul 13, 2018

Would this work as a workaround for: #6134 ?

@clintwood
Copy link

Also to consolidate the tooling for .flow and comment style typing these PRs could help facebookarchive/flow-remove-types#64 and facebookarchive/flow-remove-types#68

@steelbrain
Copy link
Contributor Author

steelbrain commented Aug 18, 2018

Tooling like that is only fixing the symptoms and not the problem. Typescript just released v3 and they still have not renamed their manifest key (typings). Backward compatibility doesn't necessarily require a new manifest key each version.

This PR has been stalled for so long. I have started building/publishing my Flow forks to use in my projects, npm users can install @steelbrain/flow-bin instead of flow-bin, binaries have also been posted on https://github.com/steelbrain/flow/releases/tag/sb-v0.79.1 https://github.com/steelbrain/flow-bin/releases/tag/sb-v0.79.1

I intend to publish future updates with this PR applied for OSX and Linux. Hopefully not for long :(

RafaelVidaurre added a commit to PolymathNetwork/polymath-apps that referenced this pull request Sep 17, 2018
due to limitations in flow (check facebook/flow#6504) this is required for now
@rgbkrk
Copy link
Contributor

rgbkrk commented Oct 19, 2018

This was the PR I held out for, in hopes this would help drive community adoption and interoperability. Our favorite libraries all have either been re-written in typescript or provide up to date definitions. The same can't be said for flow. I've loved flow too, so it saddens me quite a bit.

Thanks for the good efforts @steelbrain. It was worth a shot.

We're moving on to typescript since babel 7 makes the transition easy. nteract/nteract#3462

@sibelius
Copy link

@samwgoldman can you check this?

@mgrip
Copy link

mgrip commented Nov 2, 2018

Love that this issue is finally being addressed! One question though - doesn't this mean that modules will need to publish both the whole flow-typed and non-flow-typed versions of the codebase? (meaning twice the size the module actually needs to be). imo the ideal solution would be just having an easy way to create/flow to recognize the libdef for that module - indeed that's how typescript enables this functionality.

@Mouvedia
Copy link
Contributor

Mouvedia commented Jan 23, 2019

Will it really be analogous to the TS typings field? (i.e nothing but the types)

@goodmind
Copy link
Contributor

Any progress?

@Bannerets
Copy link

Any news?

@Hypnosphi
Copy link
Contributor

@goodmind @Bannerets please hit "Subscribe" button in the right sidebar instead of giving false alarms to those who already did.

@Bannerets
Copy link

Bannerets commented Apr 1, 2019

please hit "Subscribe" button in the right sidebar

I already subscribed before commenting.

My comment was to attract attention to stale PR, and to get status of the PR (it was a question), not for subscribing. Flow team just fully ignores this small PR for a year.
If there is a something that goes against FB plans, why can't Flow team say this?
There were many new bigger issues and PRs with much less +1s, which the devs don't just ignore.

@Mouvedia
Copy link
Contributor

Mouvedia commented Apr 1, 2019

There's a problem with the PR though.
It seems to only cover flow files (js+flow) and it's missing a key for libdef.

e.g. flow:main and flow:def or flow: { main: '', definition: '' }.

@steelbrain
Copy link
Contributor Author

I'm giving up on this because of lack of interest/support from maintainers. Moving to Typescript. Apologies to all the subscribed ones that this didn't work out :)

@steelbrain steelbrain closed this Apr 1, 2019
@goodmind goodmind added the Abandoned when PR was closed by author without resolution label Jul 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Abandoned when PR was closed by author without resolution CLA Signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet