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

apollo service:push fails because it makes assumptions about the environment #1132

Closed
ThomWright opened this issue Mar 21, 2019 · 3 comments · Fixed by #1059
Closed

apollo service:push fails because it makes assumptions about the environment #1132

ThomWright opened this issue Mar 21, 2019 · 3 comments · Fixed by #1059

Comments

@ThomWright
Copy link

ThomWright commented Mar 21, 2019

Intended outcome:

Successful apollo service:push

Actual outcome:

Failed apollo service:push

Error: EISDIR: illegal operation on a directory, read
    at Object.loadConfig (/app/node_modules/apollo-language-server/lib/config/loadConfig.js:49:50)

How to reproduce the issue:

Create an empty directory called .env in the root of the project and run apollo service:push.

Versions

apollo@2.6.2
apollo-language-server@1.5.4

Culprit

The precise problem is that apollo-language-server checks for the existence of .env, and assumes that if it exists then it is a config file it can read. In this case it is a directory. In fact, it could be a directory or a completely irrelevant file.

In general the problem is that apollo-language-server tries to make guesses about the environment it's running in. I would much prefer to configure it with explicit arguments (to the CLI, which are passed to the apollo-language-server library). I would rather it didn't look at my files, or my environment variables.

I should be able to be free to do what I want with my environment without worrying about nested dependencies looking at it and breaking.

@JakeDawkins
Copy link
Contributor

@ThomWright interesting... loading env variables from .env and similar files as well as the environment is a fairly common practice. Especially with CLI tools. I think the benefits outweigh the sacrifices pretty strongly for convenience and the ability to customize configuration in multiple ways for different dev environments/CI.

For those reasons, I don't think remove .env files or env variables is really an option at this point. Maybe we could improve the error messaging for this particular case? Or am I misunderstanding your suggestion?

@ThomWright
Copy link
Author

Well, the simple fix is to not assume that .env is a file, and instead check if it's a directory.

@JakeDawkins
Copy link
Contributor

Yeah, I think that's pretty reasonable :) regardless of what we do, we shouldn't be failing out if something's not what we expect

JakeDawkins added a commit that referenced this issue Apr 1, 2019
- Fixed bug where a user specifies loading of a config with a flag, but if that file isn't found, it loads an `apollo.config.js`. 
	- Ex: `apollo client:check --config=config/apollo.js` would also load `./apollo.config.js` if it couldn't find the specified config. I think if a user is _manually specifying_ a config, it should _only_ load what they specified and throw an error if not found. 
	- This was preventing testing errors, since leaving off a config would just load this repo's config (which wouldn't work)
- Removed the `getSearchPlaces` function. It was extraneous, and misleading. While `search places` is the term that dotenv uses, it is less descriptive than `defaultFileNames`, since that what the search places are–filenames.
- Added `try/catch` around the cosmiconfig's loader, since it can throw on malformed files with an unhelpful error.
- Handled case where `.env` is not a `File`, but a folder or something else. Resolves #1132 
- Split up project type and service name setting, since those being intertwined added unnecessary complexity
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 a pull request may close this issue.

2 participants