Skip to content
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

fix(ionic-angular): Add support for environment feature #484

Closed
wants to merge 1 commit into from

Conversation

fleboulch
Copy link
Contributor

Ionic-app-scripts introduced environment feature in v3.2.0.

Need to add types property in compilerOptions to be able to use the environment feature (as explained here: ionic-team/ionic-cli#3541)

@fleboulch fleboulch changed the title Add support for environment feature fix(ionic-angular): Add support for environment feature Oct 2, 2018
@imhoffd
Copy link
Contributor

imhoffd commented Oct 2, 2018

@florentl I'm not sure we want to put "types": [] in the starters by default, because it deviates from the default behavior of TypeScript in a way that may be unclear to those that aren't aware of it.

If anyone installs a library and then adds that library's typings (e.g. npm i -D @types/lodash), the types won't be used by TypeScript and there will be no discernible way why. But if "types": [] is added manually by them, they'll know why the behavior changed.

@imhoffd
Copy link
Contributor

imhoffd commented Oct 2, 2018

I think this should be fixed via documentation instead of providing the process declaration by default.

@fleboulch
Copy link
Contributor Author

I understand your point.
I was trying to update an ionic community starter and the travis tests failed because I introduced the environment feature. I will see what can be done to fix them.

Thanks

@imhoffd
Copy link
Contributor

imhoffd commented Oct 2, 2018

Sounds good. 👍

Closing this, but thank you for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants