Skip to content

Conversation

rpl
Copy link
Member

@rpl rpl commented Oct 5, 2016

Flow supports a new syntax for exact object types, starting from v.0.32.0:

Using the new {| prop: type |} syntax, flow checks that the objects of this type contains only exactly the defined properties, and using this additional restriction (where it is appropriate), flow will be able to catch additional classes of issues early.

This PR contains an initial proposal of the changes needed to introduce this new flowtype feature, and it is composed of:

  • a temporary change in the npm dependency to be able to use the proposed change to grunt-flowbin from fix: changed flow-bin npm package to a peer and dev dependency kumar303/grunt-flowbin#41
  • minor fixes related to flow 0.33.0 (the flowchecks were currently running on flow 0.30.0, see the above grunt-flowbin PR for a rationale)
  • "exact objects types" introduced only in src/cmd/build.js (as an initial evaluation of the impact of the change)
  • fix new flowtype validation error caught by flow in tests/unit/test-cmd/test.build.js, related to the above change in src/cmd/build.js types

@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 4be96d8 on rpl:test/flowtype-exact-objects into 508db12 on mozilla:master.

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

This is a great feature! Let's do it. Do you want to start with smaller patches like this, maybe one or two commands at a time? That might make it easier to integrate in a way that doesn't introduce too many conflicts while other development happens.

export type ExtensionManifest = {
name: string,
version: string,
applications: {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be applications? since it's optional?

Copy link
Contributor

Choose a reason for hiding this comment

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

I changed this to applications? already upstream in #639

manifestData: manifestWithoutApps,
artifactsDir: tmpDir.path(),
}, {
manifestData: manifestWithoutApps,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sold! :) This is a great catch.

@rpl
Copy link
Member Author

rpl commented Oct 6, 2016

Do you want to start with smaller patches like this, maybe one or two commands at a time?

@kumar303 exactly, that's the reason why I only did it on a single file.
my proposed plan is to turn this PR into a preparation to next "exact object types" sub-"pull requests",
with just this two changes:

Then move the commit that applies the exact object types to the src/cmd/build module (and its tests) into a new PR, and then gradually open new ones related to the other modules.

How about creating a new issue to specifically track the progress of this group of pull requests?

@kumar303
Copy link
Contributor

kumar303 commented Oct 6, 2016

The new version of grunt-flowbin is here https://www.npmjs.com/package/grunt-flowbin (I meant the version to be 0.1.0, heh, oops).

@kumar303
Copy link
Contributor

kumar303 commented Oct 6, 2016

How about creating a new issue to specifically track the progress of this group of pull requests?

Yes, that would be helpful

@kumar303
Copy link
Contributor

@shubheksha beat you to it! I believe #726 covers everything that was added here, right?

@rpl
Copy link
Member Author

rpl commented Jan 10, 2017

@kumar303 yes, definitely! 🙌

@shubheksha Thank you so much! ❤️

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.

4 participants