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

feat: added package.json as an option for configs #1266

Merged
merged 3 commits into from Mar 1, 2018

Conversation

Projects
None yet
5 participants
@ChujunGeng
Copy link
Contributor

commented Feb 21, 2018

Fixes #1166

This time I modified the loadJSConfigFile() function to parse package.json, and I also added test coverage for it.

feat: added package.json as an option for configs
added test coverage for config
@coveralls

This comment has been minimized.

Copy link

commented Feb 21, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 780839c on ChujunGeng:new/feat into 18b7243 on mozilla:master.

@kumar303
Copy link
Member

left a comment

This is looking great. I tested out the feature and it works for me.

The load order needs changing and the tests need some minor changes.

I'd also like to see this test edited to show that a package.json file will get loaded.

@@ -153,6 +157,8 @@ export async function discoverConfigFiles(
path.join(getHomeDir(), `.${magicConfigName}`),
// Look for a magic config in the current working directory.
path.join(process.cwd(), magicConfigName),
// Look for webExt key inside package.json file
path.join(process.cwd(), 'package.json'),

This comment has been minimized.

Copy link
@kumar303

kumar303 Feb 23, 2018

Member

I think we should move this up one position in the array so that it happens before path.join(process.cwd(), magicConfigName). This will make package.json load before web-ext-config.js. My rationale for this is that package.json is a project-wide type of configuration file and web-ext-config.js is a config file that is specific only to web-ext.

This comment has been minimized.

Copy link
@ChujunGeng

ChujunGeng Feb 23, 2018

Author Contributor

That makes sense. I'll change that asap.

fs.writeFileSync(
configFilePath,
`{
"name": "dummyPackage.json",

This comment has been minimized.

Copy link
@kumar303

kumar303 Feb 23, 2018

Member

technically, a version is required to make this valid so I think you should add "version": "1.0.0"

fs.writeFileSync(
configFilePath,
`{
"name": "dummyPackage.json"

This comment has been minimized.

Copy link
@kumar303

kumar303 Feb 23, 2018

Member

version should be added here too

@@ -927,6 +960,7 @@ describe('config', () => {
it('finds a config in your home directory', () => {
return withTempDir(
async (tmpDir) => {
const packageJSON = path.join(process.cwd(), 'package.json');

This comment has been minimized.

Copy link
@kumar303

kumar303 Feb 23, 2018

Member

Instead of editing this test, you should make a new one. The test you edited is specific to checking that a config in your home directory gets loaded. You need a new test specific to checking that a package.json file gets loaded. The test will probably look more like this one.

This comment has been minimized.

Copy link
@ChujunGeng

ChujunGeng Feb 23, 2018

Author Contributor

I edited this test because web-ext itself's package.json will be loaded (apparently because it's inside current working directory), and that's not expected by the original test. Unless we cd to a temporary directory, and make another temporary directory home directory to test out .web-ext-config.js, this test will always fail for the aforementioned reason.

This comment has been minimized.

Copy link
@kumar303

kumar303 Feb 23, 2018

Member

Oh, I see. It would be helpful if you can add a comment to this test explaining the situation.

@kumar303
Copy link
Member

left a comment

This feature will be really helpful. Thanks for all your work on it.

@@ -927,6 +962,10 @@ describe('config', () => {
it('finds a config in your home directory', () => {
return withTempDir(
async (tmpDir) => {
// This is actually web-ext itself's package.json file, which

This comment has been minimized.

Copy link
@kumar303

kumar303 Mar 1, 2018

Member

Thanks for this comment.

@kumar303 kumar303 merged commit 523767a into mozilla:master Mar 1, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@ChujunGeng ChujunGeng deleted the ChujunGeng:new/feat branch Mar 1, 2018

@caitmuenster

This comment has been minimized.

Copy link

commented Mar 1, 2018

Thanks so much, @ChujunGeng! Your contribution has been added to our recognition wiki.

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for your contribution!

Thanks again, and welcome onboard!

@ChujunGeng

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2018

@caitmuenster Thanks! I'm honored to join the mozillians, and I just created an account on it (https://mozillians.org/en-US/u/chujungeng/).

@caitmuenster

This comment has been minimized.

Copy link

commented Mar 2, 2018

\o/ Yay! Your profile is vouched.

We're happy to have you with us! I look forward to seeing you around the project. :)

@freaktechnik

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2018

[dev-doc-needed]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.