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

build(flow): include node_modules in flow config #702

Merged
merged 1 commit into from
Aug 22, 2018

Conversation

mrfelton
Copy link
Member

Problem / Motivation

We're missing flow type definitions for grpc. This is due to the fact that it is installed in app/node_modules and not node_modules

$ yarn flow
yarn run v1.9.2
$ flow
Launching Flow server for /Users/tom/workspace/zap-desktop
Spawned flow server (pid=76836)
Logs will go to /private/tmp/flow/zSUserszStomzSworkspacezSzZap-desktop.log
Monitor logs will go to /private/tmp/flow/zSUserszStomzSworkspacezSzZap-desktop.monitor_log
Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ app/lib/lnd/lightning.js:3:18

Cannot resolve module grpc.

     1│ // @flow
     2│
     3│ import grpc from 'grpc'
     4│ import { loadSync } from '@grpc/proto-loader'
     5│ import { BrowserWindow } from 'electron'
     6│ import LndConfig from './config'

Proposed Solution

Currently, in our .flowconfig file we specifically exclude node_modules and app/node_modules. I believe that this is left over from electron-react-boilerplate and it isn't needed.

See facebook/flow#869 for some context about why node_modules shouldn't really be excluded.

This PR adds node_modules and app/node_modules back into the flow type processing, which as well as fixing the above issue, also makes flow generally more useful as it will provide type hints for many more packages.

@mrfelton mrfelton added the type: bug 🐛 Something isn't working label Aug 22, 2018
@mrfelton mrfelton added this to the v0.2.2-beta milestone Aug 22, 2018
@mrfelton mrfelton self-assigned this Aug 22, 2018
@mrfelton mrfelton requested a review from JimmyMow August 22, 2018 11:38
@coveralls
Copy link

coveralls commented Aug 22, 2018

Pull Request Test Coverage Report for Build 3796

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 12.696%

Totals Coverage Status
Change from base Build 3793: 0.0%
Covered Lines: 485
Relevant Lines: 3350

💛 - Coveralls

@JimmyMow
Copy link
Member

Concept ACK 8c5f2bb

@JimmyMow JimmyMow merged commit 562fae2 into LN-Zap:master Aug 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants