Skip to content
Closed
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
07c1f95
Set TS base url based on node_path
robertvansteen Jan 3, 2019
ec03a78
Remove TS messages for now
robertvansteen Jan 3, 2019
ebd7650
Resole baseUrl for tsconfig/jsconfig.json
robertvansteen Jan 3, 2019
4de79d5
Add baseUrl test
robertvansteen Jan 3, 2019
839a2a3
Add jsconfig.json to kitchensink
robertvansteen Jan 3, 2019
91f7794
Add BaseUrl component
robertvansteen Jan 3, 2019
be87538
Add baseUrl as modulePath for jest
robertvansteen Jan 3, 2019
e0d3ebf
Remove NODE_PATH resolving and inform user about it’s deprecation.
robertvansteen Jan 6, 2019
5d67018
Remove unused variable
robertvansteen Jan 6, 2019
bb49cf3
Remove accident typo
robertvansteen Jan 6, 2019
f5821c2
Remove unused tests & properly load baseUrl test
robertvansteen Jan 6, 2019
832cad2
Fix wrong path to BaseUrl test
robertvansteen Jan 6, 2019
401b500
Add support for aliasing @ to src
robertvansteen Jan 6, 2019
234e361
Add integration test for alias
robertvansteen Jan 6, 2019
97ea854
Log to debug tests
robertvansteen Jan 6, 2019
2aa5605
Allow setting the baseUrl to node_modules
robertvansteen Jan 6, 2019
a0ed858
Remove unnecessary prefix for Jest module paths
robertvansteen Jan 6, 2019
e52182d
Whoops.
robertvansteen Jan 6, 2019
bf5329f
Call isValidPath method to check if the baseUrl is valid
robertvansteen Jan 6, 2019
2c19adc
Use appDirectory from paths
robertvansteen Jan 6, 2019
ac8dd39
Throw error if there are both a tsconfig.json and jsconfig.json file
robertvansteen Jan 6, 2019
a9b60b1
Add better path checking & add support for different aliases for src
robertvansteen Jan 7, 2019
637e404
Rename config to modules
robertvansteen Jan 7, 2019
830415a
Fix old reference
robertvansteen Jan 7, 2019
09d9845
Update baseUrl in Typescript compiler options
robertvansteen Jan 7, 2019
db01cb9
Log modules to debug
robertvansteen Jan 7, 2019
47b7099
Correctly return prev in reducer
robertvansteen Jan 7, 2019
50e9c60
Suffix alias with a forward slash to prevent conflicts with node_modu…
robertvansteen Jan 7, 2019
229457a
Remove console.log
robertvansteen Jan 7, 2019
216eaff
Attempt to test baseUrl for Typescript
robertvansteen Jan 7, 2019
26d6bdb
Add test to verify aliases work in TypeScript as well
robertvansteen Jan 7, 2019
74ad80a
Remove baseUrl overwrite
robertvansteen Jan 7, 2019
116c9c8
Allow setting the path of an alias to a subfolder of src
robertvansteen Jan 7, 2019
ccc6b89
Work on matching jsconfig path behavior with webpack/jest
robertvansteen Jan 9, 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
123 changes: 123 additions & 0 deletions packages/react-scripts/config/config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
// @remove-on-eject-begin
Comment thread
robertvansteen marked this conversation as resolved.
Outdated
/**
* Copyright (c) 2015-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
// @remove-on-eject-end
'use strict';

const fs = require('fs');
const path = require('path');
const chalk = require('chalk');
const paths = require('./paths');

function isValidPath(path) {
Comment thread
robertvansteen marked this conversation as resolved.
Outdated
return (
paths.relative(paths.appSrc, path) === '.' ||
paths.relative(paths.appNodeModules, path) === '.'
);
}

/**
* Get the baseUrl of a compilerOptions object.
*
* @param {Object} options
*/
function getBaseUrl(options = {}) {
Comment thread
robertvansteen marked this conversation as resolved.
Outdated
Comment thread
robertvansteen marked this conversation as resolved.
Outdated
const baseUrl = options.baseUrl;

if (!baseUrl) {
return null;
}

const baseUrlResolved = path.resolve(paths.appDirectory, baseUrl);

if (!isValidPath(baseUrlResolved)) {
console.error(
chalk.red.bold(
"You tried to set baseUrl to anything other than 'src' or 'node_modules'.This is not supported in create-react-app and will be ignored."
)
);

return null;
}

return path.resolve(paths.appDirectory, 'src');
}

/**
* Get the alias of a compilerOptions object.
*
* @param {Object} options
*/
function getAlias(options = {}) {
Comment thread
Timer marked this conversation as resolved.
Outdated
const paths = options.paths || {};

const alias = paths['@'];

const others = Object.keys(paths).filter(function(value) {
return value !== '@';
});

if (others.length) {
console.error(
chalk.red.bold(
'You tried to set one or more paths with an alias other than "@", this is currently not supported in create-react-app and will be ignored.'
)
);
}

if (!alias) {
return {};
}

if (alias.toString() !== 'src') {
console.error(
chalk.red.bold(
"You tried to set a path with alias '@' to anything other than ['src']. This is not supported in create-react-app and will be ignored."
)
);
}

return {
'@': path.resolve(paths.appDirectory, 'src'),
};
}

function getConfig() {
// Check if TypeScript is setup
const useTypeScript = fs.existsSync(paths.appTsConfig);
const hasJsConfig = fs.existsSync(paths.appJsConfig);
Comment thread
robertvansteen marked this conversation as resolved.
Outdated

if (useTypeScript && hasJsConfig) {
throw new Error(
'You have both a tsconfig.json and a jsconfig.json. If you are using Typescript please remove your jsconfig.json file.'
);
}

let config;

// If there's a tsconfig.json we assume it's a
// Typescript project and set up the config
// based on tsconfig.json
if (useTypeScript) {
config = require(paths.appTsConfig);
// Otherwise we'll check if there is jsconfig.json
// for non TS projects.
} else if (hasJsConfig) {
config = require(paths.appJsConfig);
}

config = config || {};
const options = config.compilerOptions || {};

return {
alias: getAlias(options),
baseUrl: getBaseUrl(options),
useTypeScript,
};
}

module.exports = getConfig();
22 changes: 7 additions & 15 deletions packages/react-scripts/config/env.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
'use strict';

const fs = require('fs');
const path = require('path');
const paths = require('./paths');

// Make sure that including paths.js after env.js will read .env variables.
Expand Down Expand Up @@ -48,21 +47,14 @@ dotenvFiles.forEach(dotenvFile => {
}
});

// We support resolving modules according to `NODE_PATH`.
// We used to support resolving modules according to `NODE_PATH`.
// This now has been deprecated in favor of jsconfig/tsconfig.json
// This lets you use absolute paths in imports inside large monorepos:
// https://github.com/facebook/create-react-app/issues/253.
// It works similar to `NODE_PATH` in Node itself:
// https://nodejs.org/api/modules.html#modules_loading_from_the_global_folders
// Note that unlike in Node, only *relative* paths from `NODE_PATH` are honored.
// Otherwise, we risk importing Node.js core modules into an app instead of Webpack shims.
// https://github.com/facebook/create-react-app/issues/1023#issuecomment-265344421
// We also resolve them to make sure all tools using them work consistently.
const appDirectory = fs.realpathSync(process.cwd());
process.env.NODE_PATH = (process.env.NODE_PATH || '')
.split(path.delimiter)
.filter(folder => folder && !path.isAbsolute(folder))
.map(folder => path.resolve(appDirectory, folder))
.join(path.delimiter);
if (process.env.NODE_PATH) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should move this check elsewhere imo -- probably in the script bin wrapper.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Timer in the bin wrapper the .env files are not parsed yet, so that fails to check the NODE_PATH properly. What's your concern with having it in this file?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, good point. And this just seems like an odd place to have the check (as a side effect of the file being evaluated).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we can just add it to start.js, omitting it from the build and test scripts.

console.log(
'Setting NODE_PATH to resolves modules absolutely has been deprecated in favor of setting baseUrl in jsconfig.json (or tsconfig.json if you are using Typescript) to achieve the same behaviour.'
);
}

// Grab NODE_ENV and REACT_APP_* environment variables and prepare them to be
// injected into the application via DefinePlugin in Webpack configuration.
Expand Down
3 changes: 3 additions & 0 deletions packages/react-scripts/config/paths.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ module.exports = {
appPackageJson: resolveApp('package.json'),
appSrc: resolveApp('src'),
appTsConfig: resolveApp('tsconfig.json'),
appJsConfig: resolveApp('jsconfig.json'),
yarnLockFile: resolveApp('yarn.lock'),
testsSetup: resolveModule(resolveApp, 'src/setupTests'),
proxySetup: resolveApp('src/setupProxy.js'),
Expand All @@ -106,6 +107,7 @@ module.exports = {
appPackageJson: resolveApp('package.json'),
appSrc: resolveApp('src'),
appTsConfig: resolveApp('tsconfig.json'),
appJsConfig: resolveApp('jsconfig.json'),
yarnLockFile: resolveApp('yarn.lock'),
testsSetup: resolveModule(resolveApp, 'src/setupTests'),
proxySetup: resolveApp('src/setupProxy.js'),
Expand Down Expand Up @@ -140,6 +142,7 @@ if (
appPackageJson: resolveOwn('package.json'),
appSrc: resolveOwn('template/src'),
appTsConfig: resolveOwn('template/tsconfig.json'),
appJsConfig: resolveOwn('template/jsconfig.json'),
yarnLockFile: resolveOwn('template/yarn.lock'),
testsSetup: resolveModule(resolveOwn, 'template/src/setupTests'),
proxySetup: resolveOwn('template/src/setupProxy.js'),
Expand Down
9 changes: 5 additions & 4 deletions packages/react-scripts/config/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ const paths = require('./paths');
const getClientEnvironment = require('./env');
const ModuleNotFoundPlugin = require('react-dev-utils/ModuleNotFoundPlugin');
const ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin-alt');
const config = require('./config');
console.log(JSON.stringify(config));
Comment thread
robertvansteen marked this conversation as resolved.
Outdated
const typescriptFormatter = require('react-dev-utils/typescriptFormatter');
// @remove-on-eject-begin
const getCacheIdentifier = require('react-dev-utils/getCacheIdentifier');
Expand Down Expand Up @@ -258,10 +260,7 @@ module.exports = function(webpackEnv) {
// We placed these paths second because we want `node_modules` to "win"
// if there are any conflicts. This matches Node resolution mechanism.
// https://github.com/facebook/create-react-app/issues/253
modules: ['node_modules'].concat(
// It is guaranteed to exist because we tweak it in `env.js`
process.env.NODE_PATH.split(path.delimiter).filter(Boolean)
),
modules: ['node_modules'].concat(config.baseUrl ? [config.baseUrl] : []),
// These are the reasonable defaults supported by the Node ecosystem.
// We also include JSX as a common component filename extension to support
// some tools, although we do not recommend using it, see:
Expand All @@ -275,6 +274,7 @@ module.exports = function(webpackEnv) {
// Support React Native Web
// https://www.smashingmagazine.com/2016/08/a-glimpse-into-the-future-with-react-native-for-web/
'react-native': 'react-native-web',
...config.alias,
},
plugins: [
// Adds support for installing with Plug'n'Play, leading to faster installs and adding
Expand Down Expand Up @@ -629,6 +629,7 @@ module.exports = function(webpackEnv) {
isolatedModules: true,
noEmit: true,
jsx: 'preserve',
baseUrl: config.baseUrl,
Comment thread
robertvansteen marked this conversation as resolved.
Outdated
},
reportFiles: [
'**',
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import initDOM from './initDOM';

describe('Integration', () => {
describe('jsconfig.json/tsconfig.json', () => {
it('Supports setting baseUrl to src', async () => {
const doc = await initDOM('base-url');

expect(doc.getElementById('feature-base-url').childElementCount).toBe(4);
doc.defaultView.close();
});

it('Supports setting @ as alias to src', async () => {
const doc = await initDOM('alias');

expect(doc.getElementById('feature-alias').childElementCount).toBe(4);
doc.defaultView.close();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,6 @@ describe('Integration', () => {
doc.defaultView.close();
});

it('NODE_PATH', async () => {
const doc = await initDOM('node-path');

expect(doc.getElementById('feature-node-path').childElementCount).toBe(4);
doc.defaultView.close();
});

it('PUBLIC_URL', async () => {
const doc = await initDOM('public-url');

Expand Down
8 changes: 8 additions & 0 deletions packages/react-scripts/fixtures/kitchensink/jsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"compilerOptions": {
"baseUrl": "src",
"paths": {
"@": ["src"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps add a test with * along with @* for compatibility with the previous NODE_PATH approach.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For people that had their NODE_PATH set to . and were importing like this: src/components/button?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So, I think it should be the below as a default, as that matches the NODE_PATH behaviour. Assuming the user set NODE_PATH to src would make this a good default:

  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "*": ["src/*"],
    }
  },

Then, a user could do the following, if they chose.

  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "@*": ["src/*"],
    }
  },

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure if we should provide a default as the NODE_PATH is not set to . by default which would potentially confuse users thinking that's default node behavior.

@mrmckeb mrmckeb Jan 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand, by default there's no NODE_PATH. However, I think most people that use it would use it with src.

As per most implementations that I've seen (based on Microsoft examples) you can leave "baseUrl" as ".", and the aliasing itself can happen in paths.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that makes sense and that's something we should definitely recommend in the documentation. Especially if you are migrating from NODE_PATH=src

}
}
}
9 changes: 7 additions & 2 deletions packages/react-scripts/fixtures/kitchensink/src/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,13 @@ class App extends Component {
this.setFeature(f.default)
);
break;
case 'node-path':
import('./features/env/NodePath').then(f => this.setFeature(f.default));
case 'base-url':
import('./features/config/BaseUrl').then(f =>
this.setFeature(f.default)
);
break;
case 'alias':
import('./features/config/Alias').then(f => this.setFeature(f.default));
break;
case 'no-ext-inclusion':
import('./features/webpack/NoExtInclusion').then(f =>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import React, { Component } from 'react';
import PropTypes from 'prop-types';
import load from '@/absoluteLoad';

export default class extends Component {
static propTypes = {
onReady: PropTypes.func.isRequired,
};

constructor(props) {
super(props);
this.state = { users: [] };
}

async componentDidMount() {
const users = load();
this.setState({ users });
}

componentDidUpdate() {
this.props.onReady();
}

render() {
return (
<div id="feature-alias">
{this.state.users.map(user => (
<div key={user.id}>{user.name}</div>
))}
</div>
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@

import React from 'react';
import ReactDOM from 'react-dom';
import NodePath from './NodePath';
import Alias from './Alias';

describe('NODE_PATH', () => {
describe('alias', () => {
it('renders without crashing', () => {
const div = document.createElement('div');
return new Promise(resolve => {
ReactDOM.render(<NodePath onReady={resolve} />, div);
ReactDOM.render(<Alias onReady={resolve} />, div);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export default class extends Component {

render() {
return (
<div id="feature-node-path">
<div id="feature-base-url">
{this.state.users.map(user => (
<div key={user.id}>{user.name}</div>
))}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import React from 'react';
import ReactDOM from 'react-dom';
import BaseUrl from './BaseUrl';

describe('baseUrl', () => {
it('renders without crashing', () => {
const div = document.createElement('div');
return new Promise(resolve => {
ReactDOM.render(<BaseUrl onReady={resolve} />, div);
});
});
});
2 changes: 2 additions & 0 deletions packages/react-scripts/scripts/utils/createJestConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
const fs = require('fs');
const chalk = require('chalk');
const paths = require('../../config/paths');
const jsConfig = require('../../config/config');

module.exports = (resolve, rootDir, isEjecting) => {
// Use this instead of `paths.testsSetup` to avoid putting
Expand Down Expand Up @@ -57,6 +58,7 @@ module.exports = (resolve, rootDir, isEjecting) => {
'[/\\\\]node_modules[/\\\\].+\\.(js|jsx|ts|tsx)$',
'^.+\\.module\\.(css|sass|scss)$',
],
modulePaths: jsConfig.baseUrl ? [jsConfig.baseUrl] : [],
moduleNameMapper: {
'^react-native$': 'react-native-web',
'^.+\\.module\\.(css|sass|scss)$': 'identity-obj-proxy',
Expand Down
Loading