facebook / create-react-app

Set up a modern web app by running one command.
https://create-react-app.dev
MIT License
102.77k stars 26.87k forks source link

Stop putting react-app-env.d.ts in src #6560

Open devuxer opened 5 years ago

devuxer commented 5 years ago

(This pertains to CRA w/ the --typescript flag.)

Putting a loose d.ts file at the top level of src adds clutter and breaks the convention of putting all d.ts files together in a folder, such as "types".

Suggestions to remedy this (in order of preference):

  1. Honor the new location of react-app-env.d.ts if we move it.
  2. Allow us to configure where this file goes.
  3. Place the file in src/types.

EDIT:

@nhooyr Given its confusing, you could also add a comment to the generated file explaining why its there.

@bugzpodder This file was created to include create-react-app specific types. Removing it will cause errors like:

TypeScript error in ~/src/App.tsx(2,18):
Cannot find module './logo.svg'.  TS2307

    1 | import React from 'react';
  > 2 | import logo from './logo.svg';
      |                  ^
    3 | import './App.css';
    4 | 
    5 | const App: React.FC = () => {

This was created from #5611 if you want more details.

bonavida commented 5 years ago

I'm facing the same issue and I think @devuxer's request makes sense. Are you guys going to include some fix for this in the near future?

cr4zyc4t commented 5 years ago

I'm new to typescript and very excited about official support for TS of CRA. Can anyone explain how should I use this file?

KamilPacanek commented 5 years ago

Another question from me: what is the reason behind this file? I've found out that it is generated upon starting dev server. Should I .gitignore it?

ktalebian commented 5 years ago

+1 for this; I don't understand why it is generated, and I've added it to my .gitignore

henriquepw commented 5 years ago

I fully agree

robinvdv12 commented 5 years ago

I don't see the point of this file and can't find any usefull information on the internet. +1 for this issue.

bugzpodder commented 5 years ago

It contains types for the bootstrapped App. Removing it will cause errors like:

TypeScript error in ~/src/App.tsx(2,18):
Cannot find module './logo.svg'.  TS2307

    1 | import React from 'react';
  > 2 | import logo from './logo.svg';
      |                  ^
    3 | import './App.css';
    4 | 
    5 | const App: React.FC = () => {

This was created from https://github.com/facebook/create-react-app/pull/5611 if you want more details.

bugzpodder commented 5 years ago

I don't really see this as a blocker for anyone. Having an extra file isn't ideal but it's pretty minimal by itself.

bonavida commented 5 years ago

So, it's not possible to run the bootstrapped app (with typescript) without creating this file? Maybe you should let people choose where to have it. What about tsconfig.json?

devuxer commented 5 years ago

@bugzpodder, So, one of the criteria for an issue is that it has to be a "blocker"? Perhaps that should be noted when people actually go to submit an issue, so we don't waste our own and everyone else's time.

iansu commented 5 years ago

@devuxer No, being a "blocker" is not a requirement for submitting an issue. However, following the code of conduct and being respectful to people is.

bugzpodder commented 5 years ago

To be clear I closed issue because there were some confusion as to what this file was used for and I provided an explanation. I understand the frustration that having this file might not fit your project's file structure. However I feel the proposals made in the original post all have its drawbacks. Tracking it's location is non trivial and feel like over complicating things. Having a config option for something that functionally does nothing extra is not really ideal. Forcing it in types will make other people that doesn't rely on types unhappy.

I think the feasible options are either get rid of it all together (without having broken types of course), or this idea: generate it only if type checking fails and this file doesn't exist. This might make more people happy..

devuxer commented 5 years ago

@devuxer No, being a "blocker" is not a requirement for submitting an issue. However, following the code of conduct and being respectful to people is.

@iansu, @bugzpodder, I apologize for my tone, and thank you for providing a much more detailed explanation for your decision to close the issue.

That said, I'm still a bit confused despite the extra details. Did you close this issue because (1) you felt answering a commenter's question would resolve the issue, (2) you have a better resolution in mind than my original suggestions (and plan to open a new replacement issue), or (3) you don't have the resources to fix given other priorities?

bugzpodder commented 5 years ago

This is not something we are prioritizing right now, especially given the impact is non-functional (ie not a blocker). We have tons of larger things to deal with in 3.0.2, 3.1, 3.x.

So It's either leaving the issue open indefinitely or closing it. I'd rather close it than having an indefinitely open issue and someone popping up and trying to fight stale bot. If you think you have good suggestions to this you can always open a new issue/PR and provide a more detailed proposal, which is way more productive than "not stale" comment on a stale issue.

devuxer commented 5 years ago

@bugzpodder , Fair enough. I do have one other suggestion (which I think would be easier than anything suggested so far): Create this file when all the other CRA-generated files are created, then never create it again. It's the re-appearing aspect that's really the problem (both annoying and confusing) not so much the initial location.

Edit: Also, if I'm not mistaken, none of the other CRA-generated files re-generate if I delete or move them, so this would be more consistent.

nhooyr commented 5 years ago

Given its confusing, you could also add a comment to the generated file explaining why its there.

bugzpodder commented 5 years ago

@devuxer You are right that no other CRA generated files does this. However, it is very non obvious what this file does, and removing it will cause type error. So if we don't re-generate it, then users who removed it will get a mysterious error, which no obvious way of fixing it. Maybe they'd google it, come to some stack overflow post or an issue here, find the solution deep buried in the comments.

That's why I suggested re-generate this file only if 1) there are type errors that are consistent with what this file would have prevented and 2) this file doesn't exist. I am not sure how clean the implementation would be, but it might be reasonable. Or it maybe simpler to just traverse the entire directory for this file, I don't know.

@nhooyr that definitely makes sense.

devuxer commented 5 years ago

@bugzpodder , If you follow @nhooyr 's suggestion, and include a comment that says (1) what this file is for and (2) what will happen if you delete/move it, and then cease to re-generate it, it's totally on the user at this point.

Certainly, if people Google the error, they're going to find out out pretty quickly that they need a d.ts file with one line in it to solve the problem. I think this is better than inconveniencing all the developers who know what they're doing.

bugzpodder commented 5 years ago

While this is a very reasonable proposal, it won't work for all the projects created up to this point (they won't have this comment). We'd have to either tack the comment in if the file content matches, but potentially this is still doable.

Also, potentially we can also just check for both /types/react-app-env.d.ts /react-app-env.d.ts. If there is some way of checking github repros for statistics on existence of /types in typescript code bases and say its greater than > 85%, it shouldn't be too bad to check both places.

I'd rather not have people Googling for this error if at all possible.

devuxer commented 5 years ago

@bugzpodder , I think you'll find several different conventions, unfortunately (I know I've seen "types" and "typings", and sometimes this folder is in src, other times, it's a level up in the hierarchy). The one thing that is most common is to put all such files in the same folder. If CRA wanted to be opinionated about this and specify that it must be called "types", I wouldn't complain, but others might.

nhooyr commented 5 years ago

@bugzpodder can you reopen this issue for adding the comment? People will see this file and google it and right now there is no information regarding what it does. Thats how I came upon this issue.

levrik commented 5 years ago

I moved the file to @types/react-app-env.d.ts. TypeScript seems to pick up stuff automatically from there. So, everything's working fine but every time I execute yarn start it gets re-created on the top-level. That's so annoying.

hongbo-miao commented 5 years ago

After setting up typescript-eslint, one error I got is this

src/react-app-env.d.ts 1:1 error Expected exception block, space or tab after '//' in comment spaced-comment

That is how I came here to find out what this file is.

I fixed by adding this to .eslintrc.js rules

'spaced-comment': ['error', 'always', {
  'markers': ['/'],
}],

But it would be great to just remove this file. It will make adding TypeScript ESLint experience very smooth. Thanks

aviaryan commented 5 years ago

I moved the file to @types/react-app-env.d.ts. TypeScript seems to pick up stuff automatically from there. So, everything's working fine but every time I execute yarn start it gets re-created on the top-level. That's so annoying.

As a temporary solution, I have gitignore'd src/*.d.ts and also hid it from VSCode Sidebar. I agree that this is not a critical issue but it can really be annoying for some people.

"files.exclude": {
    "src/*.d.ts": true
}
mayteio commented 4 years ago

@aviaryan I'd recommend being specific about the file to exclude if it's just this one you're annoyed by, i.e.

"files.exclude": {
  "src/react-app-env.d.ts: true
}

Otherwise your own custom type definitions will be hidden, for example when you use a package that doesn't have typings and you need to add declare module 'package-without-types'.

aviaryan commented 4 years ago

@mayteio Good point but I keep all custom typings in <rootDir>/@types so it works great for me. 😄

mayteio commented 4 years ago

That's actually a great idea. TIL

avindra commented 4 years ago

When I saw CRA popped out react-app-env.d.ts, I decided to put the /// <reference types="react-scripts" /> into an existing type declaration file in the project, as it seems really silly to have a whole file dedicated to this one line.

Doing that, everything works fine in my local build, but:

But when the build runs, it immediately breaks react-scripts test (see attached error below).

What exactly is the limitation here?

Furthermore, why is create-react-app trying to write files during the testing phase? I thought that the unit testing phase is supposed to be as pure as possible?

$ react-scripts test
internal/fs/utils.js:230
    throw err;
    ^

Error: EACCES: permission denied, open '/app/src/react-app-env.d.ts'
    at Object.openSync (fs.js:457:3)
    at Object.writeFileSync (fs.js:1282:35)
    at verifyTypeScriptSetup (/app/node_modules/react-scripts/scripts/utils/verifyTypeScriptSetup.js:257:8)
    at Object.<anonymous> (/app/node_modules/react-scripts/scripts/test.js:32:1)
    at Module._compile (internal/modules/cjs/loader.js:1158:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10)
    at Module.load (internal/modules/cjs/loader.js:1002:32)
    at Function.Module._load (internal/modules/cjs/loader.js:901:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
    at internal/main/run_main_module.js:18:47 {
  errno: -13,
  syscall: 'open',
  code: 'EACCES',
  path: '/app/src/react-app-env.d.ts'
}
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
script returned exit code 1
MariuzM commented 4 years ago

Come on CRA gives us an option to remove that file from src/ folder and place it @types, you know this is what the community wants so why ignore? CRA is an amazing app and i know you guys put a lot of effort into it, but sometimes reading the comments on what the community wants and not getting it feels like what Apple is doing with its "wall of garden".

neytema commented 4 years ago

Can this file be replaced with additional line in tsconfig.json maybe?

  "include": [
    "node_modules/react-scripts",
    "src"
  ]
bowdawn commented 4 years ago

Can this file be replaced with additional line in tsconfig.json maybe?

  "include": [
    "node_modules/react-scripts",
    "src"
  ]

did it work? the file still auto-generates with this included in my tsconfing.json file

dianjuar commented 4 years ago

I need to have it in a specific place otherwise when I run my coverage I'm getting an annoying error

image


If I put it on my typings folder everything is ok, I can compile, serve, and everything. This should respect my decision to move it.

jikkujose commented 4 years ago

Why is the React team ignoring this? Looks like its not just me who is annoyed by this. Looks like a simple fix but will make many happy.

dbeckwith commented 4 years ago

Why does this file need to be generated every time you run the dev server anyway? Could it not just be part of the template so the user is free to delete/change/move it themselves?

michielswaanen commented 3 years ago

Day 710 and still no solution for this easy fix? Hereby a friendly, cough slightly frustrated, reminder from the community! 😃

martinblostein commented 3 years ago

I'm not clear on the purpose of this file, and it's not documented anywhere.

@bugzpodder gave an explanation in 2019: "It contains types for the bootstrapped App". I don't understand what this means, what is the "bootstrapped app"? Is that just the dummy app that is included when create-react-app is initialised? In that case, why is it regenerated constantly?

They said to refer to https://github.com/facebook/create-react-app/pull/5611 for more details but that pull request is related to how this file causes problems when ejecting, it doesn't explain what the file is.

dimadk24 commented 3 years ago

You should not gitignore this file. If you import a non-js file (image, CSS module), then tsc type checker passes locally (because of the existence of react-app-env.d.ts), but fails on CI (because there's no react-app-env.d.ts) with such error: error TS2307: Cannot find module './your-file.png' or its corresponding type declarations.

So this file indeed should be committed

chris-altamimi commented 3 years ago

This is a little disappointing from the CRA team.....

It would be a little less disappointing if:

  1. It was documented somewhere clearly.
  2. We had some sort of option to move, remove this file.
  3. Create-React-App didn't rely on a basically empty file to function.....

If we are unable to ignore or less we break our app, then what in the heck do we do with this practically useless file?

jamietdavidson commented 3 years ago

Would like to reup this, I find it rather cumbersome. We are in 2021 after all :)

ghost commented 3 years ago

I like how I googled the filename to learn what it does and this discussion is one of the top results...

lunafoxfire commented 3 years ago

I'm also wondering why CRA creates this file on every run unlike every other file in the template. It's a small thing but it's really annoying from a development point of view when organization is so important. Is there a reason why adding a comment to the file describing its purpose and then letting the developer do what they want with it is too much to ask? If it's a matter of time/effort I'd be glad to do the PR myself. If it's a matter of protecting the user from borking their app, it's not really CRA's responsibility is it? I mean, the same would happen if I delete index.tsx...

SunnyTam commented 3 years ago

You should not gitignore this file. If you import a non-js file (image, CSS module), then tsc type checker passes locally (because of the existence of react-app-env.d.ts), but fails on CI (because there's no react-app-env.d.ts) with such error: error TS2307: Cannot find module './your-file.png' or its corresponding type declarations.

So this file indeed should be committed

This is true, some of the react-scripts typing are referenced from this definition file. In my case, the linting (@typescript-eslint/no-unsafe-assignment) will not work in CI. CI have no information about importing an image.

harryradford commented 1 year ago

A solution to the issue of added clutter. Hide react-app-env.d.ts from the VS Code File Explorer in settings.

  1. Go to Code > Preferences > Settings
  2. Search for "Files: Exclude"
  3. Add the src/react-app-env.d.ts pattern

Note that react-app-env.d.ts will now be excluded from fulltext searches.

thewaliii commented 7 months ago

I hate this file