Skip to content
This repository has been archived by the owner on Jan 26, 2019. It is now read-only.

Added the abillity to import js and jsx files with ts-loader #242

Merged
merged 18 commits into from
Feb 13, 2018
Merged

Added the abillity to import js and jsx files with ts-loader #242

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
9a1dc3d
added js and jsx to things that ts-loader loads
GeeWee Jan 27, 2018
11630d6
removed spaces and made prettier runnable on windows
GeeWee Jan 27, 2018
353282d
run jsx specs too
GeeWee Jan 27, 2018
9c17cd9
added isejecting
GeeWee Jan 27, 2018
edcf231
added dependencies and babeltransform
GeeWee Jan 27, 2018
0b6dac3
bumped jest version#
GeeWee Jan 27, 2018
2122eb5
does it work without cache?
GeeWee Jan 27, 2018
783abf1
cache?
GeeWee Jan 27, 2018
a1c9ebf
added ts
GeeWee Jan 27, 2018
cf4964e
updates ts-jest
GeeWee Jan 27, 2018
7ad8514
readd caching
GeeWee Jan 27, 2018
506506e
removed ts as a devdependency
GeeWee Jan 31, 2018
80c3f0d
added babel to handle js files
GeeWee Feb 4, 2018
ff4fc18
added prod env
GeeWee Feb 4, 2018
1e0a7e4
prod is handled by babel loader
GeeWee Feb 4, 2018
f9e680f
added ts as devdep
GeeWee Feb 4, 2018
5a75e0e
cache node modules
GeeWee Feb 4, 2018
69eaa28
fixed legal
GeeWee Feb 12, 2018
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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"publish": "tasks/release.sh",
"start": "node packages/react-scripts/scripts/start.js",
"test": "node packages/react-scripts/scripts/test.js --env=jsdom",
"format": "prettier --trailing-comma es5 --single-quote --write 'packages/*/*.js' 'packages/*/!(node_modules)/**/*.js'",
"format": "prettier --trailing-comma es5 --single-quote --write \"packages/*/*.js\" \"packages/*/!(node_modules)/**/*.js\"",
"precommit": "lint-staged"
},
"devDependencies": {
Expand Down
16 changes: 16 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
@@ -0,0 +1,16 @@
// @remove-file-on-eject
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just recognized: This legal information is outdated, everything is using MIT currently. Don't think its a good idea to keep this.

The original version can be found here:
https://github.com/facebook/create-react-app/blob/next/packages/react-scripts/config/jest/babelTransform.js:

/**
 * Copyright (c) 2014-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.
 */

'use strict';

const babelJest = require('babel-jest');

module.exports = babelJest.createTransformer({
presets: [require.resolve('babel-preset-react-app')],
babelrc: false,
});
4 changes: 2 additions & 2 deletions packages/react-scripts/config/webpack.config.dev.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ module.exports = {
// please link the files into your node_modules/ and let module-resolution kick in.
// Make sure your source files are compiled, as they will not be processed in any way.
new ModuleScopePlugin(paths.appSrc, [paths.appPackageJson]),
new TsconfigPathsPlugin({configFile: paths.appTsConfig})
new TsconfigPathsPlugin({ configFile: paths.appTsConfig }),
],
},
module: {
Expand Down Expand Up @@ -158,7 +158,7 @@ module.exports = {
},
// Compile .tsx?
{
test: /\.(ts|tsx)$/,
test: /\.(ts|tsx|js|jsx)$/,
include: paths.appSrc,
use: [
{
Expand Down
4 changes: 2 additions & 2 deletions packages/react-scripts/config/webpack.config.prod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ module.exports = {
// please link the files into your node_modules/ and let module-resolution kick in.
// Make sure your source files are compiled, as they will not be processed in any way.
new ModuleScopePlugin(paths.appSrc, [paths.appPackageJson]),
new TsconfigPathsPlugin({configFile: paths.appTsConfig})
new TsconfigPathsPlugin({ configFile: paths.appTsConfig }),
],
},
module: {
Expand Down Expand Up @@ -162,7 +162,7 @@ module.exports = {
},
// Compile .tsx?
{
test: /\.(ts|tsx)$/,
test: /\.(ts|tsx|js|jsx)$/,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm... I don't think it's a good idea to use ts-loader for jsx?, while using babel for the same files when transpiling the tests. Since the dependencies used in upstream CRA for transpiling those files are already included for transpiling the tests, it should be favorable to use the babel-loader for this purpose. A simple copy/paste from upstream (maybe omitting the thread-loader? Dunno when this was added...) should be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, no that's a very good point. I think we should load 'em all through ts-loader / ts-jest then. iirc ts-jest has no problems processing js. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd still be concerned about potential differences from processing them with babel - CRA uses several plugins and preset configurations to transform the code (see here), and not all of them are available using typescript (at least when not using babel v7 with its typescript transformer).
The synthesized default import that babel adds is just one of them, and projects being ported or just using js(x) files w.r.t. the official docs will definitely rely on this or any of the other transformations.

I'd favor to go the babel route for both webpack and jest due to this hardly predictable amount of potential inconsistencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if I'm understanding you right, what you're afraid of, is that the javascript code would behave different in CRA and CRA+TS ?
Because I'm reasonably sure that the typescript & JS files will behave reasonably identical between ts-jest and CRA. None of us are doing any crazy transforms on the code ( we have a babel step but that's only to hoist mock calls and stuff like that)

I don't think babel is the right way to go, we considered it as well (replacing ts-jest with just babel. See our discussion here

Copy link
Collaborator

@DorianGrey DorianGrey Jan 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So if I'm understanding you right, what you're afraid of, is that the javascript code would behave different in CRA and CRA+TS ?

At least when users are migrating or re-using components that have been created to work with CRA or similar setups, yes.
Simple example - importing react:

import React from "react";

The react module uses module.exports, thus there is no default field. As a result, this code won't compile using typescript without using allowSyntheticDefaultImports: true. However, since this flag does not affect the generated code itself, it will break at runtime, since the imported value will be undefined. babel synthesizes the default import.
To properly work with typescript, the correct way to import this would be:

import * as React from "react";

But I doubt that those who were using .js(x) files before used imports like this. That's one of the most obvious issues that might occur.
Additionally, you've already made babel-jest responsible for transpiling test files with .jsx? extension, so using babel resp. its loader is somewhat more reasonable for the build.

Don't get me wrong: I'd only take babel for .js(x) files, not for .ts(x). Thus:

test: /\.tsx?$/ => ts-loader
test: /\.jsx?$/ => babel-loader

Additionally, this would be easier to maintain, since this rule can just be copied from upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. Some valid concerns. So babel-loader for jsx files, and babel-jest (iirc is the loader that's normally used) for jest?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I suppose that's preferable.

include: paths.appSrc,
use: [
{
Expand Down
9 changes: 6 additions & 3 deletions packages/react-scripts/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
},
"dependencies": {
"autoprefixer": "7.1.6",
"babel-jest": "^22.1.0",
"babel-preset-react-app": "^3.1.1",
"case-sensitive-paths-webpack-plugin": "2.1.1",
"chalk": "1.1.3",
"css-loader": "0.28.7",
Expand All @@ -31,7 +33,7 @@
"fork-ts-checker-webpack-plugin": "^0.2.8",
"fs-extra": "3.0.1",
"html-webpack-plugin": "2.29.0",
"jest": "20.0.4",
"jest": "22.1.4",
"object-assign": "4.1.1",
"postcss-flexbugs-fixes": "3.2.0",
"postcss-loader": "2.0.8",
Expand All @@ -41,7 +43,7 @@
"source-map-loader": "^0.2.1",
"style-loader": "0.19.0",
"sw-precache-webpack-plugin": "0.11.4",
"ts-jest": "^20.0.7",
"ts-jest": "22.0.1",
"ts-loader": "^2.3.7",
"tsconfig-paths-webpack-plugin": "^2.0.0",
"tslint": "^5.7.0",
Expand All @@ -54,7 +56,8 @@
},
"devDependencies": {
"react": "^15.5.4",
"react-dom": "^15.5.4"
"react-dom": "^15.5.4",
"typescript": "^2.6.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding typescript as dev dependency is not intended - we've moved it to be a peer dependency for this package (see below), and it is added as a dev dependency by the init script when creating a new project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check it out, but Travis crashed if typescript wasn't installed and I tried to run the tests without the cache.

Internally ts-jest uses typescript as a peer-dependency, and the tests are written in typescript, so the package needs typescript as a dependency somehow. I think the reason it used to work was that Travis cached the typescript package.
Note that if something is both a devdependency and a peerDependency, it means that we'll need typescript installed, but the end-user won't have, and will instead use the peerDependency.

I'll research the travis issue a little bit more though, to make sure that we're not adding it needlessly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's currently added via the init script when a new project gets created, thus it won't be installed and added as dev dependency outside of the "regular" initialization process. I'm not sure if any of the tests is heading this way, though, but it's not impossible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean here? It doesn't seem to me like the init script is copying the devDependencies over? Only the predefined ones in 'types'

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it executes a child process that runs npm install -D resp. yarn add -D with the required @types packages and typescript in the created project folder:

create-react-app-typescript/packages/react-scripts/scripts/init.js

Lines 107 to 126 in 7149b37

const types = [
'@types/node',
'@types/react',
'@types/react-dom',
'@types/jest',
'typescript',
];
console.log(
`Installing ${types.join(', ')} as dev dependencies ${command}...`
);
console.log();
const devProc = spawn.sync(command, args.concat('-D').concat(types), {
stdio: 'inherit',
});
if (devProc.status !== 0) {
console.error(`\`${command} ${args.concat(types).join(' ')}\` failed`);
return;
}

But as mentioned above, this only reliably happens in the "regular" (i.e.: user based) workflow, and it is possible that the tests are not heading this way.

},
"peerDependencies": {
"typescript": "2.x"
Expand Down
11 changes: 7 additions & 4 deletions packages/react-scripts/scripts/utils/createJestConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const fs = require('fs');
const chalk = require('chalk');
const paths = require('../../config/paths');

module.exports = (resolve, rootDir) => {
module.exports = (resolve, rootDir, isEjecting) => {
// Use this instead of `paths.testsSetup` to avoid putting
// an absolute filename into configuration after ejecting.
const setupTestsFile = fs.existsSync(paths.testsSetup)
Expand All @@ -25,20 +25,23 @@ module.exports = (resolve, rootDir) => {
setupFiles: [resolve('config/polyfills.js')],
setupTestFrameworkScriptFile: setupTestsFile,
testMatch: [
'<rootDir>/src/**/__tests__/**/*.ts?(x)',
'<rootDir>/src/**/?(*.)(spec|test).ts?(x)',
'<rootDir>/src/**/__tests__/**/*.(j|t)s?(x)',
'<rootDir>/src/**/?(*.)(spec|test).(j|t)s?(x)',
],
testEnvironment: 'node',
testURL: 'http://localhost',
transform: {
'^.+\\.(js|jsx)$': isEjecting
? '<rootDir>/node_modules/babel-jest'
: resolve('config/jest/babelTransform.js'),
'^.+\\.tsx?$': resolve('config/jest/typescriptTransform.js'),
'^.+\\.css$': resolve('config/jest/cssTransform.js'),
'^(?!.*\\.(js|jsx|mjs|css|json)$)': resolve(
'config/jest/fileTransform.js'
),
},
transformIgnorePatterns: [
'[/\\\\]node_modules[/\\\\].+\\.(js|jsx|mjs|ts|tsx)$'
'[/\\\\]node_modules[/\\\\].+\\.(js|jsx|mjs|ts|tsx)$',
],
moduleNameMapper: {
'^react-native$': 'react-native-web',
Expand Down