Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Annotate log and defer utils #1064

Merged
merged 4 commits into from Nov 2, 2016

Conversation

AbhiGulati
Copy link
Contributor

Associated Issue: #1034

Summary of Changes

  • Add an interface file for devtools-config.js
  • Add Flow annotations for utils/defer.js and utils/log.js

Testing

  • passes npm test
  • passes npm run lint

@jasonLaster
Copy link
Contributor

Thanks @AbhiGulati, this is really great work.

You're approach to declaring the devtools-config module seems great in this case. My only question is whether we should add the declaration in a top-level interfaces dir. Do you have examples of this in other projects? One thought I had was that we could add the interface in the devtools-config package, and reference it from eslintrc. This way, other projects that install devtools-config, could get the declaration perhaps...

@jasonLaster
Copy link
Contributor

@AbhiGulati I don't know if you have time, but I'd love to get your thoughts on our type support in the redux actions.

Here's an issue for adding flow type to Actions - #1066. We've added some types to some of the other actions, but i think it's mostly low hanging fruit stuff.

I'm not really sure how to type the redux dispatch and middleware stuff. I tried some stuff here, but it was tough :)

@AbhiGulati
Copy link
Contributor Author

I'm not sure what the best place is for the interface files. I was just following tutorials that I found online. I think it would be reasonable to move it into the devtools-config module and then reference that from the flowconfig.

@AbhiGulati
Copy link
Contributor Author

I don't have any feedback on the existing type annotations or your PR; this was my first time working with Flow, and I just focused on getting the coverage up for the listed files. Have you looked into flow-typed? I think you should be able to grab type declarations for redux, but I'm not sure if it will help with thunk-middleware.

@jasonLaster
Copy link
Contributor

Oh interesting. I haven't looked at flow-typed, that could be good. You had
me fooled by the way. Thought you were a flow pro.
On Tue, Nov 1, 2016 at 6:56 PM Abhi Gulati notifications@github.com wrote:

I don't have any feedback on the existing type annotations or your PR;
this was my first time working with Flow, and I just focused on getting the
coverage up for the listed files. Have you looked into flow-typed? I think
you should be able to grab type declarations for redux, but I'm not sure if
it will help with thunk-middleware.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1064 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPiYvuwsZM42b6P-JM-fvqiHAcfCNkgks5q58N-gaJpZM4KlxGt
.

@jasonLaster
Copy link
Contributor

@AbhiGulati after thinking about it a bit more, I think the best thing to do would be to add an interface file to devtools-config and reference that in the eslintrc.

@jasonLaster jasonLaster merged commit 9a3c63b into firefox-devtools:master Nov 2, 2016
@jasonLaster
Copy link
Contributor

I moved the declaration to devtools-config.

Thanks for the help @AbhiGulati. Would love your help going forward on types or any other issues.

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.

None yet

2 participants