Allow TypeScript 3 as a peer dependency

[nickserv]

• Fix Fix peer dependencies for TS 3 #373 by updating minimum dependency versions and own own peer dependencies to allow TS 3. You may want to delete our own peer dependency entire to make this easier in the future, if TS isn't imported directly.
• Use TS 3 in development. This isn't required, but it has major improvements to errors, along with a few type system and build system features you may want to use in the future.

Verification

• yarn and yarn create-react-app no longer have peer dependency warnings for react-scripts-ts, ts-jest, and fork-ts-checker-webpack-plugin.
• CRA-TS already works without this change, it's just fixing peer dependency warnings, so this should have the same behavior ignoring peer dependencies.

[nickserv]

@DorianGrey @adambowles

[nickserv]

It looks like we're running into an unrelated Jest or JSDOM bug in CI:
localStorage is not available for opaque origins

Could someone restart the build for master to troubleshoot? I want to confirm if my PR caused this, I don't think it did.

[wmonk]

Have kicked off the build again, still failing. I know jsdom didn't support local storage at some point.

[nickserv]

Perhaps it's a regression in a newer dependency version. Because we're not using yarn lockfiles, it my be causing CI to install different dependencies on each build. Do you think we could add an older lockfile that works?

[DorianGrey]

I want to confirm if my PR caused this, I don't think it did.

Unfortunately, it did, due to the ts-jest update. There is a transitive dependency on jsdom, which became more regarding this origin handling. See: jestjs/jest#6766

For now, it's sufficient to add a specific testUrl to the jest configuration:

"testURL": "http://localhost/"

Somewhere around here: https://github.com/wmonk/create-react-app-typescript/blob/master/packages/react-scripts/scripts/utils/createJestConfig.js#L23

[nickserv]

Thanks for the tip. That config line already seems to be in createJestConfig in my branch of the fork and CRA upstream. Any ideas? Could we solve this by including JSDOM as a dependency with a minimum version, or does this affect all recent versions of JSDOM?

[DorianGrey]

Considering the reports about this issue, it seems to be side-effect of a feature that was implemented in jsdom 11.12 (localstorage, if I've read that correctly) - i.e. only including a lower version would help, but I don't think it's a good idea to step back on this.
By default, jest uses about:blank for this config entry. A PR for changing this has already been merged (jestjs/jest#6792), but not yet released.

Thus, there are two ways to solve or at least get around this issue for now:

1. Add the testURL mentioned above to the config manually.
2. Wait for a jest release including the default config update.

Given that CRA upstream already includes (2), I suppose it's favorable to pick this up as well.

[nickserv]

If I'm understanding Yarn correctly, the latest versions of packages will be installed when the package.json changes because we do not have a lockfile, so tweaking the Jest version is not an option unless we add a lockfile. testURL is already our the config. Should we wait for that PR to be released, or do you think we could add a lockfile that locks to an earlier version of Jest without this bug? It would still have to support our version of jest-dom, though.

[DorianGrey]

Sorry, I got a little bit messed up these days - lack of sleep, thanks to the unlikely high temperatures...

It's correct that testURL is part of the createJestConfig function. However, the test that currently fails on travis is defined in react-error-overlay, which uses a different jest configuration which does not yet include testURL:
https://github.com/wmonk/create-react-app-typescript/blob/master/packages/react-error-overlay/package.json#L63-L81
Adding the url there should make this test work; the others are already working fine.

[wmonk]

Do you think this could go behind #227 which essentially removes all the other packages? @DorianGrey

[nickserv]

@DorianGrey Sorry for the confusion, and thanks for the explanation! I'll try this soon.

@wmonk Alternatively we could delete the other package directories and keep the fork link, right? Git should be smart enough to keep unmodified files deleted when updates are merged from upstream, but it may try to recreate new or modified files.

[wmonk]

The main reason to get rid of the fork link is for adding upstream features from CRA which is a real pain with the extra packages. The other reason is that there are a bunch of tests that we don't care about, so can have a nice focussed test suite.

[nickserv]

If i understand correctly, the main goal is to delete sources for packages other than react-scripts. I think we can still do that by just deleting the other packages, though git might get confused about upstream merges.

…

On Tue, Aug 7, 2018, 11:18 AM Will Monk ***@***.***> wrote: The main reason to get rid of the fork link is for adding upstream features from CRA which is a real pain with the extra packages. The other reason is that there are a bunch of tests that we don't care about, so can have a nice focussed test suite. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#376 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4l9IDiqd6CNRpTqOEhW99TXc38Yzp4ks5uOa_jgaJpZM4VvX3t> .

[DorianGrey]

@wmonk : Since the affected package react-error-overlay will be removed in #227 , changing the jest config for that package would be obsolete.

Whether the config should be adopted before or not depends on which PR should be merged first. I'd suggest that this one will sooner be ready for that than #227, and contains some changes that should be merged as soon as it is ready.

[DorianGrey]

I've resolved the issues regarding jest in a different commit, which is already on master.

Currently, two tests are failing due to:

 Cannot find module 'ts-jest/preprocessor'
      3 | 'use strict';
      4 | 
    > 5 | const tsJestPreprocessor = require('ts-jest/preprocessor');
      6 | 
      7 | module.exports = tsJestPreprocessor;
      8 | 
      
      at Object.<anonymous> (../config/jest/typescriptTransform.js:5:28)

For more recent versions, this should only be ts-jest. This transformer file is used as a way to workaround some issues we had when dealing with paths - it has to work properly with and without ejection.

[wmonk]

Now that #227 is merged, this should be able to be rebased and fixed?

[nickserv]

Continued in #387.