-
-
Notifications
You must be signed in to change notification settings - Fork 27k
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
Set module resolution based on baseUrl in jsconfig/tsconfig.json #6116
Changes from 29 commits
07c1f95
ec03a78
ebd7650
4de79d5
839a2a3
91f7794
be87538
e0d3ebf
5d67018
bb49cf3
f5821c2
832cad2
401b500
234e361
97ea854
2aa5605
a0ed858
e52182d
bf5329f
2c19adc
ac8dd39
a9b60b1
637e404
830415a
09d9845
db01cb9
47b7099
50e9c60
229457a
216eaff
26d6bdb
74ad80a
116c9c8
ccc6b89
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,145 @@ | ||
// @remove-on-eject-begin | ||
/** | ||
* 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'); | ||
|
||
/** | ||
* Get the baseUrl of a compilerOptions object. | ||
* | ||
* @param {Object} options | ||
*/ | ||
function getAdditionalModulePath(options = {}) { | ||
const baseUrl = options.baseUrl; | ||
|
||
// We need to explicitly check for null and undefined (and not a falsy value) because | ||
// TypeScript treats an empty string as `.`. | ||
if (baseUrl == null) { | ||
return null; | ||
} | ||
|
||
const baseUrlResolved = path.resolve(paths.appPath, baseUrl); | ||
|
||
// We don't need to do anything if `baseUrl` is set to `node_modules`. This is | ||
// the default behavior. | ||
if (path.relative(paths.appNodeModules, baseUrlResolved) === '') { | ||
return null; | ||
} | ||
|
||
// Allow the user set the `baseUrl` to `appSrc`. | ||
if (path.relative(paths.appSrc, baseUrlResolved) === '') { | ||
return paths.appSrc; | ||
} | ||
|
||
// Otherwise, throw an error. | ||
throw new Error( | ||
chalk.red.bold( | ||
"Your project's `baseUrl` can only be set to `src` or `node_modules`." + | ||
' Create React App does not support other values at this time.' | ||
) | ||
); | ||
} | ||
|
||
/** | ||
* Get the alias of a compilerOptions object. | ||
* | ||
* @param {Object} options | ||
*/ | ||
function getAliases(options = {}) { | ||
robertvansteen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// This is an object with the alias as key | ||
// and an array of paths as value. | ||
// e.g. '@': ['src'] | ||
const aliases = options.paths || {}; | ||
|
||
return Object.keys(aliases).reduce(function(prev, alias) { | ||
// Value contains the paths of the alias. | ||
const value = aliases[alias]; | ||
|
||
// The value should be an array but we have to verify | ||
// that because it's user input. | ||
if (!Array.isArray(value) || value.length > 1) { | ||
throw new Error( | ||
chalk.red.bold( | ||
"Your project's `alias` can only be set to an array containing `src`." + | ||
' Create React App does not support other values at this time.' | ||
) | ||
); | ||
} | ||
|
||
const aliasPath = value[0]; | ||
const resolvedAliasPath = path.resolve(paths.appPath, aliasPath); | ||
|
||
if (path.relative(paths.appSrc, resolvedAliasPath) !== '') { | ||
throw new Error( | ||
chalk.red.bold( | ||
"Your project's `alias` can only be set to ['src']." + | ||
' Create React App does not support other values at this time.' | ||
) | ||
); | ||
} | ||
|
||
prev[alias] = resolvedAliasPath; | ||
return prev; | ||
}, {}); | ||
} | ||
|
||
function getJestAliases(aliases) { | ||
return Object.keys(aliases).reduce(function(prev, alias) { | ||
const aliasPath = aliases[alias]; | ||
const relativeAliasPath = path.relative(paths.appPath, aliasPath); | ||
const match = alias + '/(.*)$'; | ||
const target = '<rootDir>/' + relativeAliasPath + '/$1'; | ||
prev[match] = target; | ||
return prev; | ||
}, {}); | ||
} | ||
|
||
function getModules() { | ||
// Check if TypeScript is setup | ||
const useTypeScript = fs.existsSync(paths.appTsConfig); | ||
const hasJsConfig = fs.existsSync(paths.appJsConfig); | ||
|
||
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 || {}; | ||
|
||
const aliases = getAliases(options); | ||
const jestAliases = getJestAliases(aliases); | ||
const additionalModulePath = getAdditionalModulePath(options); | ||
|
||
return { | ||
aliases: aliases, | ||
jestAliases: jestAliases, | ||
additionalModulePath: additionalModulePath, | ||
useTypeScript, | ||
}; | ||
} | ||
|
||
module.exports = getModules(); |
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 |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"compilerOptions": { | ||
"baseUrl": "src", | ||
"paths": { | ||
"@": ["src"] | ||
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. Perhaps add a test with 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. For people that had their 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, I think it should be the below as a default, as that matches the
Then, a user could do the following, if they chose.
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. Hmm, I'm not sure if we should provide a default as 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 understand, by default there's no NODE_PATH. However, I think most people that use it would use it with As per most implementations that I've seen (based on Microsoft examples) you can leave 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. Yes, that makes sense and that's something we should definitely recommend in the documentation. Especially if you are migrating from |
||
} | ||
} | ||
} |
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 |
---|---|---|
@@ -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); | ||
}); | ||
}); | ||
}); |
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.
We should move this check elsewhere imo -- probably in the script bin wrapper.
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.
You mean this file? https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/bin/react-scripts.js
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.
Yup!
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.
@Timer in the bin wrapper the
.env
files are not parsed yet, so that fails to check theNODE_PATH
properly. What's your concern with having it in this file?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.
Ah, good point. And this just seems like an odd place to have the check (as a side effect of the file being evaluated).
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.
Maybe we can just add it to
start.js
, omitting it from thebuild
andtest
scripts.