Skip to content
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

feat: allow custom jsxImportSource #10583

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+32 −2
Open

feat: allow custom jsxImportSource #10583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3251212
feat: allow custom jsxImportSource
hermansje Feb 20, 2021
54d5ecf
feat: prevent inconsistent jsxImportSource
hermansje Feb 20, 2021
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/babel-preset-react-app/create.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,13 @@ module.exports = function (api, opts, env) {
// Adds component stack to warning messages
// Adds __self attribute to JSX which React will use for some warnings
development: isEnvDevelopment || isEnvTest,
runtime: opts.runtime || 'classic',
// Will use the native built-in instead of trying to polyfill
// behavior for any plugins that require one.
...(opts.runtime !== 'automatic' ? { useBuiltIns: true } : {}),
runtime: opts.runtime || 'classic',
...(opts.runtime === 'automatic' && opts.importSource != null
? { importSource: opts.importSource }
: {}),
},
],
isTypeScriptEnabled && [require('@babel/preset-typescript').default],
Expand Down
2 changes: 2 additions & 0 deletions packages/react-scripts/config/jest/babelTransform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@ const hasJsxRuntime = (() => {
return false;
}
})();
const jsxImportSource = process.env.JSX_IMPORT_SOURCE;

module.exports = babelJest.createTransformer({
presets: [
[
require.resolve('babel-preset-react-app'),
{
runtime: hasJsxRuntime ? 'automatic' : 'classic',
...(jsxImportSource ? { importSource: jsxImportSource } : {}),
},
],
],
Expand Down
4 changes: 4 additions & 0 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 @@ -86,6 +86,7 @@ const hasJsxRuntime = (() => {
return false;
}
})();
const jsxImportSource = process.env.JSX_IMPORT_SOURCE;

// This is the production and development configuration.
// It is focused on developer experience, fast rebuilds, and a minimal bundle.
Expand Down Expand Up @@ -414,6 +415,9 @@ module.exports = function (webpackEnv) {
require.resolve('babel-preset-react-app'),
{
runtime: hasJsxRuntime ? 'automatic' : 'classic',
...(jsxImportSource
? { importSource: jsxImportSource }
: {}),
},
],
],
Expand Down
23 changes: 22 additions & 1 deletion packages/react-scripts/scripts/utils/verifyTypeScriptSetup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ function verifyTypeScriptSetup() {
if (appTsConfig.compilerOptions == null) {
appTsConfig.compilerOptions = {};
firstTimeSetup = true;
}
}

for (const option of Object.keys(compilerOptions)) {
const { parsedValue, value, suggested, reason } = compilerOptions[option];
Expand Down Expand Up @@ -250,6 +250,27 @@ function verifyTypeScriptSetup() {
}
}

if (parsedCompilerOptions.jsxImportSource != null) {
if (hasJsxRuntime && semver.gte(ts.version, '4.1.0-beta')) {
if (process.env.JSX_IMPORT_SOURCE == null) {
process.env.JSX_IMPORT_SOURCE = parsedCompilerOptions.jsxImportSource;
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this shouldn't set this global but rather it should be passed around using normal arguments etc. Assigning to a global pollutes the global env and might be surprising - the relation between the files involved in this becomes much more implicit and much less obvious.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for taking a look at this @Andarist!
I know, I'm not a big fan either. I wrote it this way because an environment variable is used to control the related JSX transform (DISABLE_NEW_JSX_TRANSFORM), because it enables the feature for JavaScript users and because it made a first version easy to implement 🙂

An alternative implementation, at least for the TypeScript use cases, would be extracting the parsing of the tsconfig.json file and passing the result to the verify function and (a part of it) as an extra argument to configFactory.

Based on feedback on this PR, I can change the implementation to one that the maintainers are happy with.

Copy link
Contributor

Choose a reason for hiding this comment

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

The usage of process.env.JSX_IMPORT_SOURCE is not that bad - as you have mentioned, some other stuff is handled in a similar manner. The problem (from my PoV) is that you also write to a process.env and not only read from it.

Also - I'm just a bystander here, so it might be best to wait for maintainers' feedback before pursuing anything that I suggest 😉

}
} else {
appTsConfig = immer(appTsConfig, config => {
delete config.compilerOptions.jsxImportSource;
});
messages.push(
`${chalk.cyan(
'compilerOptions.jsxImportSource'
)} removed (requires ${chalk.bold(
'babel react runtime'
)} to be ${chalk.cyan.bold('automatic')} and ${chalk.bold(
'typescript'
)} version ${chalk.cyan.bold('>=4.1.0-beta')})`
);
}
}

// tsconfig will have the merged "include" and "exclude" by this point
if (parsedTsConfig.include == null) {
appTsConfig = immer(appTsConfig, config => {
Expand Down