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

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

Merged
merged 18 commits into from Feb 13, 2018

Conversation

4 participants
@GeeWee
Contributor

GeeWee commented Jan 27, 2018

This PR adds the abillity for ts-loader to also process js and jsx files, and lets Jest pick them up as well.

This is good progress for if you want to migrate a large js app gradually to typescript

I've verified it via running it on a mixed repository:
image

and tests:
image

I'm not sure whether there's a smarter way to test it. I did it by editing the node_modules folder directly in this branch https://github.com/GeeWee/flatris/tree/react-scripts-ts-test

(if you want to test, fetch repo, run yarn install, and then git reset --hard to override)

Closes #230 and #217

(I also changed the prettier config in package.json as single quotes within double quotes doesn't work on windows)

@GeeWee

This comment has been minimized.

Contributor

GeeWee commented Jan 27, 2018

I battled a little bit with Travis Turns out that I had to upgrade Jest, but Travis was caching the old version. I disabled the cache but then it started failing on Typescript which is only listed as a peerDependency, and not as a devDependency. I think the old tests might have worked because the typescript dependency was cached.

Anyway, I've updated the jest and ts-jest dependencies and added typescript as a devDependency.

If possible perhaps it should be possible to clear the jenkins cache for people with PRs? I don't have much experience to see if that's possible.

@foyangyu

This comment has been minimized.

foyangyu commented Jan 29, 2018

I spend all night but didn't find any clue.
Is there a right way to import jsx class from .js to .tsx file?
Really need some help buddy. The problem as follows.
Microsoft/TypeScript#14558

@GeeWee

This comment has been minimized.

Contributor

GeeWee commented Jan 29, 2018

I don't think this is the right place for that :)

@foyangyu

This comment has been minimized.

foyangyu commented Jan 30, 2018

You're right, haha

@@ -162,7 +162,7 @@ module.exports = {
},
// Compile .tsx?
{
test: /\.(ts|tsx)$/,
test: /\.(ts|tsx|js|jsx)$/,

This comment has been minimized.

@DorianGrey

DorianGrey Jan 30, 2018

Collaborator

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.

This comment has been minimized.

@GeeWee

GeeWee Jan 31, 2018

Contributor

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?

This comment has been minimized.

@DorianGrey

DorianGrey Jan 31, 2018

Collaborator

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.

This comment has been minimized.

@GeeWee

GeeWee Jan 31, 2018

Contributor

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

This comment has been minimized.

@DorianGrey

DorianGrey Jan 31, 2018

Collaborator

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.

This comment has been minimized.

@GeeWee

GeeWee Feb 1, 2018

Contributor

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?

This comment has been minimized.

@DorianGrey

DorianGrey Feb 1, 2018

Collaborator

Yes, I suppose that's preferable.

@@ -54,7 +56,8 @@
},
"devDependencies": {
"react": "^15.5.4",
"react-dom": "^15.5.4"
"react-dom": "^15.5.4",
"typescript": "^2.6.2"

This comment has been minimized.

@DorianGrey

DorianGrey Jan 30, 2018

Collaborator

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.

This comment has been minimized.

@GeeWee

GeeWee Jan 31, 2018

Contributor

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.

This comment has been minimized.

@DorianGrey

DorianGrey Jan 31, 2018

Collaborator

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.

This comment has been minimized.

@GeeWee

GeeWee Jan 31, 2018

Contributor

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'

This comment has been minimized.

@DorianGrey

DorianGrey Feb 1, 2018

Collaborator

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:

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.

@GeeWee

This comment has been minimized.

Contributor

GeeWee commented Jan 31, 2018

@DorianGrey please note the latest failing build. The only difference in my commit was disabling the cache and removing typescript.

@DorianGrey

This comment has been minimized.

Collaborator

DorianGrey commented Jan 31, 2018

If this causes too many problems, I'm fine with adding it as a dev dependency as well.

@GeeWee

This comment has been minimized.

Contributor

GeeWee commented Feb 4, 2018

note that (https://travis-ci.org/wmonk/create-react-app-typescript/jobs/337258268) is after typescript has been re-added as a devdependency, cache still disabled and it works. I think adding is the right choice.

@GeeWee

This comment has been minimized.

Contributor

GeeWee commented Feb 4, 2018

We now use the babel loader for js files in both jest and the regular webpack configuration @DorianGrey (do you have twitter btw? I've noticed you around the typescript scene and I'm a fan)

* 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.
*/

This comment has been minimized.

@DorianGrey

DorianGrey Feb 6, 2018

Collaborator

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.
 */
@DorianGrey

This comment has been minimized.

Collaborator

DorianGrey commented Feb 6, 2018

LGTM, with the exception of the legal information in the babelTransform.js, please update this, and it'll be ready for merge.
Since these are 17 commits now, squashing them would be useful, otherwise I'll do that on merge.

And no, I don't have twitter account - don't have enough time to maintain (semi-)social media accounts.

@GeeWee

This comment has been minimized.

Contributor

GeeWee commented Feb 12, 2018

I've changed the license to the standard MIT license found in the rest of the files.

Feel free to squash the commits on merge.

@DorianGrey

This comment has been minimized.

Collaborator

DorianGrey commented Feb 13, 2018

LGTM, thanks!

@DorianGrey DorianGrey merged commit ec0a39e into wmonk:master Feb 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@speerhead

This comment has been minimized.

speerhead commented Mar 9, 2018

This will be great for migrating my js app to Typescript. How can I determine what version of react-scripts-ts this makes it in?

@GeeWee

This comment has been minimized.

Contributor

GeeWee commented Mar 21, 2018

@speerhead this has been released

eanders-MS pushed a commit to Microsoft/BotFramework-Emulator that referenced this pull request Apr 27, 2018

William Wong William Wong
Merged PR 4076: Revisit all package.json and lerna setup
# Changelog
* Put `devDependencies` to root, fixed `package-lock.json`
  * Removed unnecessary `devDependencies`
  * Please use `lerna add` from now on, don't use `npm install` or edit `package.json`
    * To add `jest` as `devDependencies` to `botframework-emulator-shared` package
    * At root, run `npm install jest --save-dev` followed by `lerna add jest --dev --scope=botframework-emulator-shared`
  * Please commit `package-lock.json` if you have any changes
    * Lerna [bug](lerna/lerna#1290) may cause unnecessary changes in `package-lock.json`, `#master` or `@3.0.0` will fix it
    * Until their next drop, please verify/tweak your changes in `package-lock.json` before committing the file
* Bumped all dependencies to latest version
  * We are on latest `typescript@2.7.2` 🎉
  * `electron@1.8.1`
  * `restify@4.3.2` (@6 now, but their plugins architecture changed)
* Scoping internal packages with `@bfemulator` for CI/CD on our NPM feed
  * `/app/client` (`botframework-emulator-client`) is now `@bfemulator/client`
  * `/app/shared` (`botframework-emulator-shared`) is now `@bfemulator/app-shared`
* Revisited all `.gitignore` files
* Added `npm test` scripts to all `package.json`, by default, use `jest`
* Temporarily added `react-app-rewired` to mix JSX in TypeScript React app (broken since `react-scripts-ts@>2.9.0`)
  * [Tracking bug](wmonk/create-react-app-typescript#266) at `react-scripts-ts`, their [next drop](wmonk/create-react-app-typescript#242) should have it

# Notes
Added few TypeScript ignores because:

* Some typings are wrong (added `skipLibCheck`)
* Some typings are outdated (using `any` type)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment