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

Adds support for es6 named import, overall cleanup and fixes #188

Merged
merged 5 commits into from
Jul 11, 2016
Merged

Adds support for es6 named import, overall cleanup and fixes #188

merged 5 commits into from
Jul 11, 2016

Conversation

alexeygolev
Copy link
Contributor

Almost a complete rewrite. While cleaning up for named import I decided to write tests in a more manageable way. Turned out quite a few of the typings had problems with currying. Also now with union types updates in 0.28 (hence the version requirement update) I was able to be quite specific with types (as usual — flow keeps impressing me). Fantasy-land and lenses are still not there. I need to wrap my head around typing it properly as it was created with dynamic types in mind.
I think the typings file is actually easier to navigate so I hope it's now more convenient to improve the typings or fix bugs.

@mwalkerwells
Copy link
Contributor

Thanks for your work on this! Going to upgrade my project tonight. 😀

@ryyppy
Copy link
Contributor

ryyppy commented Jul 11, 2016

This confuses me... is there no export keyword anymore? Are all function declarations inside a declare module scope automatically assumed to be public interfaces?

@alexeygolev
Copy link
Contributor Author

alexeygolev commented Jul 11, 2016

@ryyppy yep:)
otherwise for ramda I would have to write things twice to be able to export default ramda with all the methods and each method as a named export

/* @flow */
/* eslint-disable no-unused-vars, no-redeclare */

type Transformer<A,B> = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to leave this old libdef in place for anyone using older versions of Flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, ramda actually gains a lot from union types rewrite. To the point that there is no use for some typings with older versions.
Another thing. The tests are not version based — my new tests won't pass with the old version

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, ramda actually gains a lot from union types rewrite. To the point that there is no use for some typings with older versions.

It's certainly true that the unions fixes make things a lot better -- but given that at least some people found this libdef useful pre-v0.28, wouldn't you agree that having a best-possible libdef for that version is better than no libdef at all?

The tests are not version based — my new tests won't pass with the old version

You can move test files into the flow_>=v0.23.x_<=v0.27.x directory and that will ensure that it only runs for that particular version of the libdef.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. will do now:)

@jeffmo
Copy link
Contributor

jeffmo commented Jul 11, 2016

This confuses me... is there no export keyword anymore? Are all function declarations inside a declare module scope automatically assumed to be public interfaces?

I kind of predicted this confusion (and I have plans to address it -- but it involves deprecating stuff which will take time), but here's the context:

If the body of a declare module has no declare export statements or if it has a declare module.exports: ...;, then it is interpreted as a CommonJS module. Historically (pre-v0.28) you could only write declare modules that were interpreted as CommonJS modules. It wasn't until the declare export support in v0.28 that Flow would interpret declare modules as ES modules (if, and only if, they have a declare export statement in them).

Additionally, historically, every declare you'd have in a declare module would be considered a property on the exports object. This is still the case for CommonJS declare modules for backward compatibility.

So because this libdef uses no declare export statements, Flow infers this libdef as declaring a CommonJS module and thus all the old behavior still applies.

My thoughts on addressing this confusion:

I'd like to add a new declare cjs-module "Foo" {} statement and deprecate the use of declare module for CommonJS module libdefs. This will serve 2 purposes:

(1) Make it less implicit which modules are ES and which are CommonJS.
(2) It will give us the opportunity to require that declare module.exports: ...; is specified (rather than implicit exports like the legacy behavior).

Once both forms exist for a while (something like 4-5 releases) we can add a deprecation notice and eventually remove support for using declare module to declare CommonJS modules at all, and we'll be in a world where all exports are explicit and the kind of the module being declared is also explicit.

@jeffmo
Copy link
Contributor

jeffmo commented Jul 11, 2016

It just occurred to me that moving the old tests into the old libdef directory makes them not run for new libdefs, but having the new tests still sitting at the toplevel will still run them against the old libdef as well.

So I think you'll need to move the new tests into the flow_>=v0.28.x_<=v0.29.x directory as well

@@ -0,0 +1,760 @@
/* @flow */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason you upper-bounded this libdef at 0.29? Might as well leave it unbounded until the tests fail, eh?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we have a monorepo it's then possible that somebody won't be able to push something if ramda fails on a newer version.

Copy link
Contributor

@jeffmo jeffmo Jul 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Yea, this bit us with 0.28.

I'd actually like to solve this at the tooling level when we deploy new versions of Flow. I'd like to make a quick little script that just runs all tests and bounds their libdefs at the new version of Flow. Then we can just run this right as we deploy each new version of Flow.

I think this will be important to make sure people aren't missing out on all the libdefs in the repo that would actually still work every time a new version of Flow goes out.

I'm going to talk with the rest of the team about it today, but in the meantime I'm happy to take on the responsibility of adding upper bounds with each of the upcoming Flow deploys.

@alexeygolev
Copy link
Contributor Author

finally managed to make it pass))))

@jeffmo jeffmo merged commit c513a3d into flow-typed:master Jul 11, 2016
@ryyppy ryyppy mentioned this pull request Aug 19, 2016
cullophid pushed a commit to cullophid/flow-typed that referenced this pull request Jun 19, 2017
* Added fix to prevent arrays being mutated into objects when recursuvely merging

* Amended 'recursiveMerge' test title to better reflect what is happening

* Added an object check so that we don't try and merge non-objects
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

4 participants