read from .env by default in inject() #70
Conversation
test/tools/environment.js
Outdated
| tools.env.inject(); | ||
| process.env.PIZZA.should.equal('Blackjack'); | ||
| should.not.exist(process.env.CITY); | ||
| done(); |
There was a problem hiding this comment.
The tests are synchronous code, so I guess we don't need done() here. So are the other test cases in this file.
test/tools/environment.js
Outdated
|
|
||
| describe('read env', () => { | ||
| describe.only('read env', () => { |
There was a problem hiding this comment.
Should we remove .only here? Seems mocha only runs the tests in test/tools/environment.js because of this.
There was a problem hiding this comment.
oh good catch. that was just for me, since I didn't want to run the whole suite every time. Will remove.
| @@ -31,10 +31,29 @@ const cleanEnvironment = () => { | |||
| } | |||
| }; | |||
|
|
|||
| const localFilepath = filename => { | |||
| return path.join(process.cwd(), filename || ''); | |||
There was a problem hiding this comment.
Not sure if it's valid use case: Could a user inject an environment file using an absolute path? For example:
const zapier = require('zapier-platform-core');
zapier.tools.env.inject('/absolute/path/to/environment/file');If so, we may want to adjust a bit here because path.join doesn't seem to handle absolute paths:
> process.cwd()
'/Users/eliang/Projects/zapier-platform/core'
> path.join(process.cwd(), '/root')
'/Users/eliang/Projects/zapier-platform/core/root'
There was a problem hiding this comment.
they couldn't, but they also couldn't before. In master, we resolved their filename to the local directory anyway. We could expand that, but that seems like a more disruptive change (instead of renaming, they have to pass the whole path in).
| '\nWARNING: `.environment` files will no longer be read by default in the next major version.', | ||
| 'Either rename your file to `.env` or explicitly call this function with a filename:', | ||
| '\n zapier.tools.env.inject(".environment");\n\n' | ||
| ].join('\n') |
There was a problem hiding this comment.
We may want to hide this when running tests.
|
Cleaned up in fd59bb7! |
Bumped the dotenv dependency, hence the path changes. Fixes PDE-76.