-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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 flow typechecking as a webpack plugin #1152
Changes from 51 commits
10ec304
c7eacf8
b0c39e7
de16269
31b2e04
d4763a5
2f34a67
79b3b86
d004f2d
ec8d230
bb39b19
8878502
6a5cfc1
513c0a9
719ac82
c76f15e
ba7a05a
2362346
f83fa6a
f42c5cc
f9ae1f0
c5e2102
e4e2111
f9bf33d
4448e98
db929a1
eac318d
054357f
b3e5609
cabeadb
fa6d3c8
05ae460
e30a959
3ed5f2c
026e581
6e44124
3047218
09c2c13
3949fb3
159638b
5105c29
4fa4569
ba483c6
a48e8c3
9e1957e
c834095
f5d3982
cfc1115
030f7c1
37444f2
08ca783
51cca10
7e33f9d
a3a78f7
eb9e341
137f59f
2fd2dbc
7c5749d
f4299ef
a3fb2df
de99610
29628ef
30494dc
f850266
8d1f515
1df2cb5
70ad8dd
45841cd
dab6f9d
1cb808e
e94387c
45e6c29
fb4735c
800af94
ad625f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,243 @@ | ||
var fs = require('fs'); | ||
var path = require('path'); | ||
var chalk = require('chalk'); | ||
var childProcess = require('child_process'); | ||
var flowBinPath = require('flow-bin'); | ||
const flowTypedPath = path.join(__dirname, 'runFlowTyped.js'); | ||
|
||
function stripFlowLoadingIndicators(message) { | ||
var newMessage = message; | ||
var launchingIndex = newMessage.indexOf("Launching Flow server for"); | ||
if (launchingIndex >= 0) { | ||
newMessage = newMessage.slice(0, launchingIndex); | ||
} | ||
var stillIndex = newMessage.indexOf("flow is still initializing"); | ||
if (stillIndex >= 0) { | ||
newMessage = newMessage.slice(0, stillIndex); | ||
} | ||
var notRespIndex = newMessage.indexOf("The flow server is not responding"); | ||
if (notRespIndex >= 0) { | ||
newMessage = newMessage.slice(0, notRespIndex); | ||
} | ||
return newMessage; | ||
} | ||
|
||
function execOneTime(command, args, options) { | ||
return new Promise((resolve, reject) => { | ||
var stdout = new Buffer(""); | ||
var stderr = new Buffer(""); | ||
var oneTimeProcess = childProcess.spawn( | ||
command, | ||
args, | ||
options | ||
); | ||
oneTimeProcess.stdout.on('data', chunk => { | ||
stdout = Buffer.concat([stdout, chunk]); | ||
}); | ||
oneTimeProcess.stderr.on('data', chunk => { | ||
stderr = Buffer.concat([stderr, chunk]); | ||
}); | ||
oneTimeProcess.on('error', error => reject(error)); | ||
oneTimeProcess.on('exit', code => { | ||
switch (code) { | ||
case 0: | ||
return resolve(stdout); | ||
default: | ||
return reject(new Error( | ||
Buffer.concat([stdout, stderr]).toString() | ||
)); | ||
} | ||
}); | ||
}); | ||
} | ||
|
||
function writeFileIfDoesNotExist(path, data) { | ||
return new Promise((resolve, reject) => { | ||
fs.exists(path, exists => { | ||
if (!exists) { | ||
fs.writeFile(path, data, err => { | ||
if (err) { | ||
return reject(err); | ||
} | ||
resolve(true); | ||
}); | ||
} else { | ||
resolve(false); | ||
} | ||
}); | ||
}); | ||
} | ||
|
||
function writeInFileIfNotPresent(path, contentToAssert, contentToAppend) { | ||
return new Promise((resolve, reject) => { | ||
fs.exists(path, exists => { | ||
if (!exists) { | ||
fs.writeFile(path, contentToAppend, err => { | ||
if (err) { | ||
return reject(err); | ||
} | ||
resolve(true); | ||
}); | ||
} else { | ||
fs.readFile(path, (err, existingContent) => { | ||
if (err) { | ||
return reject(err); | ||
} | ||
if (existingContent.indexOf(contentToAssert) < 0) { | ||
fs.appendFile(path, contentToAppend, err => { | ||
if (err) { | ||
return reject(err); | ||
} | ||
resolve(true); | ||
}); | ||
} else { | ||
resolve(false); | ||
} | ||
}); | ||
} | ||
}); | ||
}); | ||
} | ||
|
||
function getFlowVersion(options) { | ||
return execOneTime( | ||
(options || {}).global ? "flow" : flowBinPath, | ||
['version', '--json'] | ||
) | ||
.then(rawData => JSON.parse(rawData)) | ||
.then(versionData => versionData.semver); | ||
} | ||
|
||
function initializeFlow(projectPath, flowconfig, otherFlowTypedDefs) { | ||
const flowconfigPath = path.join(projectPath, '.flowconfig'); | ||
const gitignorePath = path.join(projectPath, '.gitignore'); | ||
return getFlowVersion().then(localVersion => | ||
getFlowVersion({global: true}).catch(() => localVersion) | ||
.then(globalVersion => | ||
globalVersion !== localVersion ? | ||
Promise.reject(new Error( | ||
'Flow integration was disabled because the global Flow version does not match.\n' + | ||
'You may either remove the global Flow installation or install a compatible version:\n' + | ||
' ' + chalk.cyan('npm') + ' install -g flow-bin@' + localVersion | ||
)) : | ||
localVersion | ||
) | ||
) | ||
.then(localVersion => Promise.all([ | ||
writeFileIfDoesNotExist(flowconfigPath, flowconfig.join('\n')) | ||
.then(wroteFlowconfig => wroteFlowconfig ? | ||
writeInFileIfNotPresent(gitignorePath, 'flow-typed', 'flow-typed/npm') : | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we need to not do this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because it should always use the local version? I'm trying to wrap my head around the remaining issues to see if I can help get this merged 🙃 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See this comment: #1152 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks Dan! I dropped the lines that added flow-typed to the gitignore. Now I'm not completely sure what the protocol is for changes on a PR, but since it looked like a new PR is my only option, that's what I did: |
||
false | ||
), | ||
execOneTime( | ||
flowTypedPath, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if Can you provide me with a strong argument why a flow user can't scaffold this themselves? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, developing in the package seems to behave incorrectly:
I would expect this under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the alternative to using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if there is an alternative but there's not many packages in the repository anyway. It's no where near what DefinitelyTyped contains. I vote we remove it unless there's a compelling reason people wouldn't do it themselves. And I believe James just said we're using it wrong anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Timer It'll eventually have all of DefinitelyTyped inside as well as ones that were better vetted. I would not remove flow-typed from the repository– that would be a mistake. All you need to do is remove it from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thejameskyle okay, thanks for the information! Sounds like it's staying then. Thanks. :) |
||
['install', '--overwrite', '--flowVersion=' + localVersion], | ||
{ cwd: projectPath } | ||
) | ||
// This operation will fail if react-scripts is a path to a tarball in the | ||
// package.json (like in End To End testing!). So we swallow this error. | ||
// See https://github.com/flowtype/flow-typed/issues/399#issuecomment-266766678 | ||
.catch((e) => /(invalid comparator)|(unable to rebase the local cache repo)/i.test(e.message) ? true : Promise.reject(e)) | ||
.then(() => Promise.all( | ||
Object.keys(otherFlowTypedDefs).map((packageName) => execOneTime( | ||
flowTypedPath, | ||
[ | ||
'install', | ||
packageName + '@' + otherFlowTypedDefs[packageName], | ||
'--overwrite', | ||
'--flowVersion=' + localVersion | ||
], | ||
{ cwd: projectPath } | ||
)) | ||
)) | ||
])); | ||
} | ||
|
||
function flowCheck(projectPath) { | ||
return execOneTime( | ||
flowBinPath, | ||
['status', '--color=always'], | ||
{ cwd: projectPath } | ||
); | ||
} | ||
|
||
function FlowTypecheckPlugin(options) { | ||
options = options || {}; | ||
// Contents of the generated .flowconfig if it doesn't exist | ||
this.flowconfig = options.flowconfig || []; | ||
// Load up other flow-typed defs outside of the package.json (implicit packages behind react-scripts) | ||
// Key is the package name, value is the version number | ||
this.otherFlowTypedDefs = options.otherFlowTypedDefs || {}; | ||
} | ||
|
||
FlowTypecheckPlugin.prototype.apply = function(compiler) { | ||
var flowActiveOnProject = false; | ||
var flowInitialized = false; | ||
var flowInitError = null; | ||
var flowInitializationPromise; | ||
var flowShouldRun = false; | ||
var flowErrorOutput = null; | ||
|
||
// During module traversal, assert the presence of an @ flow in a module | ||
compiler.plugin('compilation', (compilation, params) => { | ||
compilation.plugin('normal-module-loader', (loaderContext, module) => { | ||
// We're only checking the presence of flow in non-node_modules | ||
// (some dependencies may keep their flow comments, we don't want to match them) | ||
if (module.resource.indexOf("node_modules") < 0) { | ||
// We use webpack's cached FileSystem to avoid slowing down compilation | ||
loaderContext.fs.readFile(module.resource, (err, data) => { | ||
if (data && data.toString().indexOf('@flow') >= 0) { | ||
if (!flowActiveOnProject) { | ||
flowInitializationPromise = (!compiler.parentCompilation ? | ||
initializeFlow( | ||
compiler.options.context, this.flowconfig, this.otherFlowTypedDefs | ||
) : Promise.resolve() | ||
) | ||
.then(() => { | ||
flowInitialized = true; | ||
}, e => { | ||
flowInitError = e; | ||
return Promise.reject(e); | ||
}); | ||
flowActiveOnProject = true; | ||
} | ||
flowShouldRun = true; | ||
} | ||
}); | ||
} | ||
}) | ||
}); | ||
|
||
// While emitting, run a flow check if flow has been detected | ||
compiler.plugin('emit', (compilation, callback) => { | ||
// Only if a file with @ flow has been changed | ||
if (flowShouldRun) { | ||
flowShouldRun = false; | ||
(flowInitialized ? | ||
(flowInitError ? Promise.reject(flowInitError) : Promise.resolve()) : | ||
flowInitializationPromise) | ||
.then( | ||
() => flowCheck(compiler.options.context), | ||
e => Promise.reject(e) // don't run a check if init errored, just carry the error | ||
) | ||
.then(() => { | ||
flowErrorOutput = null; | ||
compilation.flowPassed = true; | ||
}, error => { | ||
flowErrorOutput = stripFlowLoadingIndicators(error.message); | ||
compilation.warnings.push(flowErrorOutput); | ||
}) | ||
.then(callback); | ||
} else { | ||
// Output a warning if flow failed in a previous run | ||
if (flowErrorOutput) { | ||
compilation.warnings.push(flowErrorOutput); | ||
} else { | ||
compilation.flowPassed = true; | ||
} | ||
callback(); | ||
} | ||
}); | ||
}; | ||
|
||
module.exports = FlowTypecheckPlugin; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
#!/usr/bin/env node | ||
var flowTyped = require('flow-typed'); | ||
|
||
flowTyped.runCLI(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ var HtmlWebpackPlugin = require('html-webpack-plugin'); | |
var CaseSensitivePathsPlugin = require('case-sensitive-paths-webpack-plugin'); | ||
var InterpolateHtmlPlugin = require('react-dev-utils/InterpolateHtmlPlugin'); | ||
var WatchMissingNodeModulesPlugin = require('react-dev-utils/WatchMissingNodeModulesPlugin'); | ||
var FlowTypecheckPlugin = require('react-dev-utils/FlowTypecheckPlugin'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All the other imports uses |
||
var getClientEnvironment = require('./env'); | ||
var paths = require('./paths'); | ||
|
||
|
@@ -226,7 +227,13 @@ module.exports = { | |
// to restart the development server for Webpack to discover it. This plugin | ||
// makes the discovery automatic so you don't have to restart. | ||
// See https://github.com/facebookincubator/create-react-app/issues/186 | ||
new WatchMissingNodeModulesPlugin(paths.appNodeModules) | ||
new WatchMissingNodeModulesPlugin(paths.appNodeModules), | ||
// Trigger some typechecking if a file matches with an @ flow comment | ||
new FlowTypecheckPlugin({ | ||
otherFlowTypedDefs: { | ||
jest: "17.0.0" | ||
} | ||
}) | ||
], | ||
// Some libraries import Node modules but don't use them in the browser. | ||
// Tell Webpack to provide empty mocks for them so importing them works. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,6 +94,12 @@ function setupCompiler(host, port, protocol) { | |
|
||
if (isSuccessful) { | ||
console.log(chalk.green('Compiled successfully!')); | ||
if (messages.isFlowIntegrationErrorMessage) { | ||
console.log(chalk.yellow('Flow checks were skipped.')); | ||
} | ||
if (stats.compilation.flowPassed) { | ||
console.log(chalk.green('Flow checks have passed.')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if saying that flow checks have passed is particularly useful. Looks like noise in my output, especially since it shows up even when I haven't decorated anything with Can we just remove this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I imagined that Flow users unaware of the extent of integration might think it didn't run. However, they'd probably make errors often enough to notice those are being reported. So maybe you're right. |
||
} | ||
} | ||
|
||
if (showInstructions) { | ||
|
@@ -131,6 +137,12 @@ function setupCompiler(host, port, protocol) { | |
console.log('You may use special comments to disable some warnings.'); | ||
console.log('Use ' + chalk.yellow('// eslint-disable-next-line') + ' to ignore the next line.'); | ||
console.log('Use ' + chalk.yellow('/* eslint-disable */') + ' to ignore all warnings in a file.'); | ||
console.log('Use ' + chalk.yellow('// $FlowFixMe') + ' to ignore flow-related warnings on the next line.'); | ||
} | ||
|
||
// We print why flow was disabled | ||
if (messages.isFlowIntegrationErrorMessage) { | ||
console.log(messages.isFlowIntegrationErrorMessage); | ||
} | ||
}); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does global flow version matter?