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

Allow async config file exports #9

Merged
merged 2 commits into from
Sep 23, 2020

Conversation

TehShrike
Copy link
Contributor

This would allow config files to be CommonJS files that pull in connection details from ESM files using dynamic imports.

Alternately, util.local could be changed to be async, and use dynamic imports if the file extension is .mjs or something, but forcing config files to be CJS while allowing asynchrony seemed less problematic.

This allows config files to be CommonJS files that pull in connection details from ESM files using dynamic imports
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2020

Codecov Report

Merging #9 into master will decrease coverage by 2.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
- Coverage   48.07%   45.94%   -2.14%     
==========================================
  Files           2        2              
  Lines         104      111       +7     
==========================================
+ Hits           50       51       +1     
- Misses         54       60       +6     
Impacted Files Coverage Δ
index.js 11.76% <0.00%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c97dc45...6c37920. Read the comment docs.

@lukeed
Copy link
Owner

lukeed commented Sep 22, 2020

I think I'm good with this. Just to make sure we're on the same page, can you share a demo config file that'd use this?

@TehShrike TehShrike closed this Sep 22, 2020
@TehShrike TehShrike deleted the allow-async-config-files branch September 22, 2020 21:47
@lukeed
Copy link
Owner

lukeed commented Sep 22, 2020

Accident? 🤔

@TehShrike
Copy link
Contributor Author

oh shit, yeah, apparently I accidentally wiped this out while going push-crazy on the command-line

@TehShrike TehShrike restored the allow-async-config-files branch September 22, 2020 22:20
@TehShrike TehShrike reopened this Sep 22, 2020
@TehShrike
Copy link
Contributor Author

Here's a config file I'm imagining:

module.exports = import('./environment.mjs').then(({ db_user, db_password }) => ({
	host: 'localhost',
	database: 'my_sweet_app',
	user: db_user,
	pass: db_password
}))

I tested this change locally by just using Promise.resolve e.g.

module.exports = Promise.resolve({
	host: 'localhost',
	database: 'my_sweet_app',
	user: 'testuser',
	pass: 'somepass'
})

@lukeed
Copy link
Owner

lukeed commented Sep 23, 2020

I don't really know why but those examples give me the heebie jeebies haha. It feels a bit off to do module.exports = Promise, even though module.exports = async function () {} is exactly the same thing and feels fine to me. 🤷

@TehShrike
Copy link
Contributor Author

hah, yeah, I can understand that. Taste could easily drive you to

module.exports = async () => {
	const { db_user, db_password } = await import('./environment.mjs')

	return {
		host: 'localhost',
		database: 'my_sweet_app',
		user: db_user,
		pass: db_password
	}
}

I put the await on the outside because I figured allowing either version was the reasonable thing to do.

@lukeed lukeed merged commit 5b6aaf0 into lukeed:master Sep 23, 2020
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.

3 participants