Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add typings for process.env #5557

Merged
merged 1 commit into from
Oct 24, 2018
Merged

Conversation

brunolemos
Copy link
Contributor

@brunolemos brunolemos commented Oct 24, 2018

Babel doesn't support namespaces like export namespace App {}, but it does seem to support export declare namespace App {} or extending an existing namescape inside a declaration file (.d.ts) like we are doing here. Everything seems to work fine for this use case. (related)

image

@Timer
Copy link
Contributor

Timer commented Oct 24, 2018

What about user defined REACT_APP_ vars? Can we fall back to { [key: string]: string | undefined }?
Also, test is a valid env.

@Timer Timer added this to the 2.1 milestone Oct 24, 2018
@Timer
Copy link
Contributor

Timer commented Oct 24, 2018

Does this merge friendly if the user installs @types/node?

@brunolemos brunolemos force-pushed the typescript-process-env branch from 6433064 to f688010 Compare October 24, 2018 17:19
@brunolemos
Copy link
Contributor Author

brunolemos commented Oct 24, 2018

What about user defined REACT_APP_ vars? Can we fall back to { [key: string]: string | undefined }? Also, test is a valid env.

Added.

Does this merge friendly if the user installs @types/node?

Yes, just tested and worked as normal. It's because this code is extending an existing type, not creating or replacing completely

@Timer
Copy link
Contributor

Timer commented Oct 24, 2018

I dunno what process.browser is for, we def don't support that. If we do, it's by accident and isn't covered by our semver guarantee. Can we remove that type please?

@brunolemos brunolemos force-pushed the typescript-process-env branch from f688010 to adf5693 Compare October 24, 2018 17:39
@brunolemos
Copy link
Contributor Author

brunolemos commented Oct 24, 2018

@Timer removed. It's added by webpack, it will probably be always true with cra because there's no ssr support right

@Timer
Copy link
Contributor

Timer commented Oct 24, 2018

@brunolemos ah, yeah, webpack is an implementation detail and we regularly bump major versions of webpack in patches. One day, we might not even use webpack -- so I don't want to rely on this.

@brunolemos brunolemos changed the title Add typings for process.browser and process.env Add typings for process.env Oct 24, 2018
@brunolemos brunolemos force-pushed the typescript-process-env branch from adf5693 to 15f2308 Compare October 24, 2018 17:44
@Timer Timer merged commit 68a3735 into facebook:master Oct 24, 2018
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Oct 29, 2018
nate770 pushed a commit to ONTW/create-react-app that referenced this pull request Oct 30, 2018
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants