Skip to content

Workspaces typescript fixes #3

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Dec 23, 2019
Merged

Workspaces typescript fixes #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4b7c4fb
Remove unused functions
pmoleri Jul 15, 2019
c8c9853
Don't consider a subpackage as the workspace root
pmoleri Jul 15, 2019
2dec991
Add 'browser' and 'module' package main fields
pmoleri Jul 15, 2019
19e2490
Find react-scripts even when hoisted
pmoleri Jul 16, 2019
a754bcf
Set up ForkTsCheckerWebpackPlugin for workspaces
pmoleri Jul 16, 2019
e78111b
Disable typescript linting outside main project
pmoleri Jul 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/react-scripts/config/paths.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
const path = require('path');
const fs = require('fs');
const url = require('url');
const findUp = require('find-up');

// Make sure any symlinks in the project folder are resolved:
// https://github.com/facebook/create-react-app/issues/637
Expand Down Expand Up @@ -122,7 +123,9 @@ module.exports = {
};

const ownPackageJson = require('../package.json');
const reactScriptsPath = resolveApp(`node_modules/${ownPackageJson.name}`);
const reactScriptsPath = findUp.sync(`node_modules/${ownPackageJson.name}`, {
cwd: resolveApp('.'),
});
const reactScriptsLinked =
fs.existsSync(reactScriptsPath) &&
fs.lstatSync(reactScriptsPath).isSymbolicLink();
Expand Down
39 changes: 26 additions & 13 deletions packages/react-scripts/config/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,26 @@ module.exports = function(webpackEnv) {
const isEnvDevelopment = webpackEnv === 'development';
const isEnvProduction = webpackEnv === 'production';

const workspacesMainFields = [workspacesConfig.packageEntry, 'main'];
const workspacesMainFields = [
workspacesConfig.packageEntry,
'browser',
'module',
'main',
];
const mainFields =
isEnvDevelopment && workspacesConfig.development
? workspacesMainFields
: isEnvProduction && workspacesConfig.production
? workspacesMainFields
: undefined;

const includePaths =
isEnvDevelopment && workspacesConfig.development
? [paths.appSrc, ...workspacesConfig.paths]
: isEnvProduction && workspacesConfig.production
? [paths.appSrc, ...workspacesConfig.paths]
: paths.appSrc;

// Webpack uses `publicPath` to determine where the app is being served from.
// It requires a trailing slash, or the file assets will get an incorrect path.
// In development, we always serve from the root. This makes config easier.
Expand Down Expand Up @@ -342,11 +354,13 @@ module.exports = function(webpackEnv) {
loader: require.resolve('eslint-loader'),
},
],
include: isEnvDevelopment && workspacesConfig.development
? [paths.appSrc, workspacesConfig.paths]
: isEnvProduction && workspacesConfig.production
? [paths.appSrc, workspacesConfig.paths]
: paths.appSrc,
include: includePaths,
// Don't lint typescript files outside the main package because it has problems with some syntax rules, e.g. abstract
exclude: useTypeScript
? file =>
/\.tsx?/.test(path.extname(file)) &&
!file.startsWith(paths.appSrc)
: undefined,
},
{
// "oneOf" will traverse all following loaders until one will
Expand All @@ -368,12 +382,7 @@ module.exports = function(webpackEnv) {
// The preset includes JSX, Flow, TypeScript, and some ESnext features.
{
test: /\.(js|mjs|jsx|ts|tsx)$/,
include:
isEnvDevelopment && workspacesConfig.development
? [paths.appSrc, workspacesConfig.paths]
: isEnvProduction && workspacesConfig.production
? [paths.appSrc, workspacesConfig.paths]
: paths.appSrc,
include: includePaths,
loader: require.resolve('babel-loader'),
options: {
customize: require.resolve(
Expand Down Expand Up @@ -656,6 +665,10 @@ module.exports = function(webpackEnv) {
typescript: resolve.sync('typescript', {
basedir: paths.appNodeModules,
}),
compilerOptions: {
skipLibCheck: true,
suppressOutputPathCheck: true,
},
async: isEnvDevelopment,
useTypescriptIncrementalApi: true,
checkSyntacticErrors: true,
Expand All @@ -667,7 +680,7 @@ module.exports = function(webpackEnv) {
'!**/src/setupProxy.*',
'!**/src/setupTests.*',
],
watch: paths.appSrc,
watch: includePaths,
silent: true,
// The formatter is invoked directly in WebpackDevServerUtils during development
formatter: isEnvProduction ? typescriptFormatter : undefined,
Expand Down
19 changes: 7 additions & 12 deletions packages/react-scripts/config/yarn-workspaces.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,13 @@ const getWorkspacesRootConfig = dir => {

const packageObj = loadPackageJson(packageJsonUp);

if (Reflect.has(packageObj, 'workspaces')) {
if (
packageObj.workspaces &&
(
Array.isArray(packageObj.workspaces) ||
Reflect.has(packageObj.workspaces, 'packages')
)
) {
const workspacesRootConfig = {
root: path.dirname(packageJsonUp),
workspaces: packageObj.workspaces
Expand Down Expand Up @@ -161,17 +167,6 @@ const getDeps = pkg => {

const depsTable = {};

const filterDeps = deps =>
Reflect.ownKeys(deps).filter(dep => Reflect.has(depsTable, dep));

const filterDepsTable = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering why you thought to remove this?

My best guess is that you didn't see it doing anything for your project, and that you were thinking it was not needed?

Hopefully it was not breaking anything for you in Typescript?

The reason I added, this was so the dev-server does not lint workspace dependencies that were not added to the package.json file of the app that is being launched. I think you'll find that without these lines, all of your worspaces with main:src in the package.json will be linted when launching your React app from the dev-server, even if the dependencies never get used by the app.

The way we are using react-workspaces we currently have 7 apps, and 13 suites of components. Linting 13 suites of components for 7 apps takes a couple minutes; but it takes about 10 seconds for one app. As the monorepo grows, linting unused code gets increasingly expensive, so we would like to avoid this cost.

Interested to get your thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, thanks for explaining the rationale, I think watch and linting are working as expected.
I just removed them because they seem to be currently unused in the in master. I only noticed it because VS Code shows them grayed out.

Reviewing the code again... I'll make my own guess :)
I think the functions were renamed and both versions were kept:

filterDeps -> filterSrcPaths 
filterDepsTable -> buildDepsTable

and old versions can be safeley removed.
Does that seem right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I see what you mean. I will double check this. Thanks!

Reflect.ownKeys(depsTable).forEach(depName => {
const depsList = depsTable[depName].deps;
const workspacesOnlyDeps = filterDeps(depsList);
depsTable[depName].deps = workspacesOnlyDeps;
});
};

const buildDepsTable = srcPaths => {
srcPaths.forEach(path => {
const pkg = getPkg(path);
Expand Down