Merge react-scripts@2.x.x upstream #409
Conversation
|
4.0.7 or 4.0.8 ? |
|
4.0.8. I've been publishing with the next tag so you can always check that |
|
@wmonk, here's my insight so far. As you can see, the bundle size has decreased. Also, the traspiled code kicks in fine down to IE11. My However, now I get some annoying deprecation messages, which I believe come from outdated webpack dependencies |
|
Thanks @jkomyno! The deprecation warnings are annoying I agree, will get them fixed before the final release. Otherwise is everything working well? |
|
Everything else works well. |
| "ts-jest": "22.0.1", | ||
| "ts-loader": "^2.3.7", | ||
| "ts-loader": "4.x.x", |
There was a problem hiding this comment.
I noticed some "BREAKING CHANGES" here https://github.com/TypeStrong/ts-loader/blob/master/CHANGELOG.md#500
There was a problem hiding this comment.
That should not be a problem in our case - the breaking change refers to a change in the generation of declaration files.
There was a problem hiding this comment.
ts-loader maintainer here; you should switch to v5. @DorianGrey is correct 😄
| "ts-loader": "4.x.x", | |
| "ts-loader": "5.x.x", |
| "ts-jest": "22.0.1", | ||
| "ts-loader": "^2.3.7", | ||
| "ts-loader": "4.x.x", | ||
| "tsconfig-paths-webpack-plugin": "^2.0.0", | ||
| "tslint": "^5.7.0", |
There was a problem hiding this comment.
Please update it as well (5.11.0).
| "react": "^15.5.4", | ||
| "react-dom": "^15.5.4", | ||
| "react": "^16.3.2", | ||
| "react-dom": "^16.3.2", | ||
| "typescript": "3.0.1" |
There was a problem hiding this comment.
I guess that we need to use 3.0.3 or maybe 3.1.1 … Wdyt?
There was a problem hiding this comment.
Recommending ~3.1. The changes in 3.1 are especially extending the evaluation depth and precision of nested and intersected generics, which causes the compiler to find even more type errors than before (and thus avoid even more potential runtime errors).
|
Hey @wmonk! PS: Thanks a lot for your job. |
|
@r3nya thanks for your input! Have updated the description! |
|
@wmonk I guess that we need to add "exclude": [
…
"src/setupTests.ts",
"src/setupProxy.js"
]Because we have Update: it's only for |
| "sw-precache-webpack-plugin": "0.11.4", | ||
| "style-loader": "0.23.0", | ||
| "terser-webpack-plugin": "1.1.0", | ||
| "thread-loader": "1.2.0", | ||
| "ts-jest": "22.0.1", |
There was a problem hiding this comment.
Please update it as well. Latest version is 23.10.2.
| 'json', | ||
| 'node', | ||
| 'mjs', | ||
| ], | ||
| globals: { | ||
| 'ts-jest': { | ||
| tsConfigFile: paths.appTsTestConfig, |
There was a problem hiding this comment.
When you will update ts-jest, you need to update this line to
"tsConfig": paths.appTsTestConfig|
Faced with little issue after ejecting.
"globals": {
"ts-jest": {
"tsConfigFile": "/Users/r3nya/…/tsconfig.test.json"
}
},I've checked code and I guess that we can hardcode relative path for You need to …
globals: {
'ts-jest': {
tsConfigFile: 'tsconfig.test.json',
},
},or if you will update globals: {
'ts-jest': {
tsConfig: 'tsconfig.test.json',
},
}, |
| transpileOnly: true, | ||
| poolTimeout: Infinity, // keep workers alive for more effective watch mode | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This loader has most recently been removed again: facebook/create-react-app#5170
One of the more complicated loaders to properly set up in general, and does not have that much benefit in most cases with a properly set up caching mechanism in particular.
| options: { | ||
| importLoaders: 1, | ||
| poolTimeout: Infinity, // keep workers alive for more effective watch mode | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Same about thread-loader here.
|
|
||
| // First, run the linter. | ||
| // It's important to do this before Babel processes the JS. |
There was a problem hiding this comment.
Which linter does this comment refer to ? The eslint block from CRA is not present here, thus the comment is somewhat misleading.
| // We need to use our own loader until `babel-loader` supports | ||
| // customization | ||
| // https://github.com/babel/babel-loader/pull/687 | ||
| loader: require.resolve('babel-loader'), |
There was a problem hiding this comment.
Same here regarding thread-loader.
| use: [ | ||
| // This loader parallelizes code compilation, it is optional but | ||
| // improves compile time on larger projects | ||
| require.resolve('thread-loader'), |
There was a problem hiding this comment.
... and the 4th but last time on this loader.
| } | ||
| to { | ||
| transform: rotate(360deg); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Is there a particular reason why this file differs from CRA ? Their version is not currently using css modules.
| </header> | ||
| <p className="App-intro"> | ||
| To get started, edit <code>src/App.tsx</code> and save to reload. | ||
| </p> | ||
| </div> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| export default App; |
There was a problem hiding this comment.
Same question for the reason of the difference here.
Additionally, CSS modules are a bit more complicated to use in TS without a proper type declaration. There are special loaders for this, like: https://github.com/olegstepura/typed-css-modules-loader
But apparently, this causes additional overhead to integrate properly.
There was a problem hiding this comment.
I think it would be in the spirit of this package to provide a css modules loader. Since CRA provides a CSS Modules loader in its config by default, this would be part of maintaining feature parity, but with TS. I tried this one, but it didn't seem to generate the correct typings or at least I couldn't figure it out in the time I had. The important thing is that the regexes for modules be separate, like .module.css and .module.scss, so that users using regular SASS or CSS don't experience the module loader's overhead.
There was a problem hiding this comment.
A question above is just about the example App code, which differs from CRA.
In the current state of the config, it would be possible to use CSS modules (using the extensions you mentioned), it's just that specific typings would be generated; instead, contents of these files will be typed as a generic any for now. I'm not that familiar with the available loaders or plugins to achieve more specific ones at the moment, esp. what they generate and where. However, it's work a closer look and see if we can get this working.
There was a problem hiding this comment.
I highly do not recommend adding typed-css-modules-loader or any css typing support to this. I've gone down this road recently and only hit hell when dealing with CSS being imported from 3rd party node_modules that have special characters that will make any typing parser spectacularly explode. It's better to leave css loaders as typed as any imho.
| @@ -19,26 +19,23 @@ const isLocalhost = Boolean( | |||
| ) | |||
| ); | |||
|
|
|||
| export default function register() { | |||
| export default function register(config: any) { | |||
There was a problem hiding this comment.
The config parameter deserves a bit more strict type declaration, doesn' it?
Currently, it's just an object with optional onUpdate and onSuccess parameters, which both receive a service worker registration object if available.
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| function registerValidSW(swUrl: string) { | ||
| function registerValidSW(swUrl: string, config: any) { |
There was a problem hiding this comment.
Same about the config object here.
| }; | ||
| }) | ||
| .catch(error => { | ||
| console.error('Error during service worker registration:', error); | ||
| }); | ||
| } | ||
|
|
||
| function checkValidServiceWorker(swUrl: string) { | ||
| function checkValidServiceWorker(swUrl: any, config: any) { |
| "description": "Configuration and scripts for Create React App.", | ||
| "repository": "wmonk/create-react-app", | ||
| "license": "BSD-3-Clause", | ||
| "repository": "wmonk/react-scripts-ts", |
There was a problem hiding this comment.
|
Me or my team will try to integrate this this week, we have a non-ejected production app (I know!) built with CRATS that will be a good test to try and upgrade. |
|
I Tried this update (from 2.17.0 to 4.0.8) but, just after the change, I got an import issue (that I didnt have before). From a file withPaging.hoc.tsx :
From file index.ts in the same directory I got the error on npm start :
We investigate on this issue but, just before the upgrade of react-scripts-ts we didn't have it |
|
Works even great with react-app-rewired. |
In this case, if we move the exported interfaces in other file ( it looks like a problem webpack but I have not found any issue for that... |
|
I got also some issues with Apollo Client who doesnt work anymore
It seems there was same issues with CRA2 : |
|
Is this upstreamed to react-scripts@2.0.3? I really would like the improved code-splitting support that this version added. |
|
CRA now supports TypeScript out of the box. facebook/create-react-app#4837 |
|
@mucsi96 Wow, did not even know that something like that is being considered and they worked on it for a couple of months already. That will surely make things much easier. I am going to give it a try to see possible gotchas and if this fork is still needed for some reason. |
|
It seems that even though some support for Typescript has been added it's still painful to configure. If you look at the first comment in hacker news on CRA2, full Typescript support is still in the waiting list: This might also justify why they don't have anything about it in their new official website: |
|
@alexandrudanpop The PR for TypeScript support in CRA2 has merged, but it hasn't been released yet. The HN comment is referring to the officially released CRA2 (which indeed still has no TypeScript support). I'm testing it today to see how it feels, but the setup is just installing the TypeScript related npm packages and creating a tsconfig.json. |
|
The CRA2 is not using Typescript compiler but babel-preset-typescript. There is a more information in this article (especially caveats section). The paragraph about type checking is not true because there is use of |
|
Hi everyone, thanks for your comments. Is there any reason to carry on with this, or is it all covered by CRA with it's new TS support? |
|
I still prefer |
|
@wmonk well, do they also handle linting and running tests on ts files (besides type checking which seems to be covered/ or not?!)? If they do, I guess there's probably no point to continue. |
|
@wmonk I think it's clear that CRA will get the most usage now, even if it's missing functionality available in this project, as it will get added step by step. However, as an existing user of |
|
@wmonk I think that it's definitely worth continuing with this given that CRA 2 will not support TS path mappings (see facebook/create-react-app#5585). |
|
Since I am already using rewire for some other stuff, adding tsconfig-paths-webpack-plugin into the mix is nothing too serious. What bothers me more is that transpilation is done with Babel alone and not by TypeScript compiler. I would rather have a choice of what version of TypeScript I am going to use than relying on Babel maintainers to get a new stuff in there reliably enough. Even though in most cases it's about stripping type information, I do recall how long it took to Babel to just implement a shorter variant of Also I don't want to do typechecking separately. Even though it takes longer time to build the app, I like that it runs all type checking during it so I can fix issue right away. Sometimes even VSCode does not sees some issues reliably. So yea, I think it's still worth maintaining this fork and I am going to be using it for sure. |
|
Ok, it seems that people will still get value from this fork. As it will still be maintained, but different to the CRA implementation, I am happy to add new features that would diverge away from the original intention of CRA. |
|
@wmonk Question remains how to actually deal with the fact, that CRA now contains configuration for |
|
TypeScript is now officially supported as of Create React App 2.1. Read the release notes to get started. |
|
@Timer It doesn't feel right from you to announce it like some holy grail in this repo since it's not 100% support. Not talking about relative paths only now, but also the fact that CRA is not using TypeScript compiler and does not do type checking. These facts are enough for this fork to still exists. |
I'm sorry you feel that way, however, the support is going to be fine for most individuals and organizations. Supporting aliasing is a footgun and we'll be adding absolute import support soon. These features are marginally useful because VSCode and other IDEs automatically import and update paths as you move files. Also, Create React App performs type checking using your installed version of TypeScript, so there must be a misunderstanding. |
I mostly came to this understanding from the page about plugin-transform-typescript.
However, for some reason, I've missed this in setup docs.
Honestly, reading this sentence I would not understand it anyway. What console? It's a somewhat confusing statement. Would be much better to simply say, that code will by type checked during the build. Well, those path aliases are big enough obstacle for me anyway. It's a shame really, such a small thing and yet very important. |
|
@Timer I would like to try your new release out myself. Is there documentation for migrating from this package to your new release? (to see if I should go with the original CRA or with CRA-ts) |
|
Sure @maracuja-juice! Here's an excellent blog post: https://vincenttunru.com/migrate-create-react-app-typescript-to-create-react-app/ |
|
@Timer Well, so I tried to create fresh CRA2.0 and then out of curiosity added
I don't think this is cool at all touching the config I've modified. It should be my own risk doing something unsupported and not force me to something else. Oh well, back to this fork then. |
|
@FredyC if you want a custom setup you can always eject |
|
@wilcoschoneveld Seriously? Because of one small thing to give up on all future updates? No thanks :) This is also kinda important in regards to this fork. I am wondering what's the stance of @wmonk on this. Are you going to keep this "we know best what your tsconfig should look like" behavior? :) |
Not giving up "one small thing" is already preventing you from getting updates. You're using a fork that's always behind on features. We weighed the value of having
You're more than welcome to use this fork. In fact, I think having this fork is healthy for users that want a different experience than Create React App (this repo has never closely followed the CRA philosophy). I'm here merely to inform users that there's now an alternate choice. We're very grateful for all the work @wmonk has done maintaining this fork. |
|
Hey everyone, just want to weigh in again. I think it could be cool to carry on with this fork of CRA and add all the bells and whistles that TS (and the ecosystem) provides, which might not be suited to the original philosophy of CRA. I was always keen to track what CRA was doing, and so put off various suggestions in favour of that. I am happy to make react-scripts-ts a more configurable environment now, rather than following the (useful) restrictions CRA puts in place. In terms of getting this PR shipped, and starting to accept new feature suggestions, I currently don't have an ETA. I know that isn't super helpful, but work is busy enough that I don't have the time right now to do all the work. Please subscribe for updates when I get around to merging this. Thanks! |
|
@Timer you've already clearly informed users that there is an alternate choice in your initial post (which I thought was fine), and you've followed it up by declaring that CRA is the authority on what is correct and incorrect, and users should go back and deal with it in its current state, which contradicts the purpose of a fork. I'm not sure what contribution you've made to this PR aside from your initial post (and of course, co-authoring CRA!). I would personally appreciate focusing future responses here on the contents of this pull request and what it aims to achieve within the context of CRA-TS. There are plenty of other channels to communicate with users who are interested in CRA. |
This is a first pass at merging in
react-scripts@2.x.x. You can try this out by installing version4.0.7:create-react-app test --scripts-version=react-scripts-ts@4.0.8I'd like to get some decent feedback on these changes to make sure it's working properly for existing projects. So please chime in!