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.

Sign up for GitHub

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

Jump to bottom

Envinfo config file #176

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Envinfo config file #176

wants to merge 4 commits into from

Conversation

ryhinchey
Copy link
Collaborator

This PR adds support for an envinfo.js configuration file. Based on the discussion here #158.

Some notes on the design considerations from @tabrindle:

size. envinfo is frequently used via npx, so I want to preserve small size for speed of download

  • the implementation doesn't add enough code to make a material impact on the package's size IMO

the file having a consistent name and extension. (extensionless dotfiles bother me)

  • the only supported file is envinfo.js

ability to use a remote config file (fetched via http, possibly a future feature)

  • you can use Promises in the envinfo.js file. See the tests added as part of this PR

the contents of the config file being similar or identical to config passed inline when using the library directly in js

  • they're identical

somehow declaring compatible versions of config vs versions of envinfo

  • I didn't touch on this and am not entirely sure what this would entail but I'd be happy to take a stab at it as part of this PR or a future one

@tabrindle
Copy link
Owner

This is awesome - I'll take a deeper look at it tomorrow, but as of now, my only suggestion is that we use the more conventional naming of envinfo.config.js

webpack and babel both do it this way, and I think that's probably a bandwagon we should jump on.

@tabrindle tabrindle changed the base branch from master to main June 15, 2020 00:03
tabrindle
tabrindle requested changes Jun 23, 2020
View reviewed changes
src/envinfo.js Outdated
@@ -83,9 +83,20 @@ function main(props, options) {
});
}

function runMainFromConfigFile(configFile, options) {
return Promise.resolve(configFile()).then(({ options: configOptions, ...config }) => {
return main(config, { ...options, ...configOptions });
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you switch the order of options and configOptions, I think we can use command line flags to override whats in the config file, I think this is ideal

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also be able to merge command line switches for --system as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tabrindle I switched the order of options and configOptions which makes a lot of sense.

For merging command line switches, shouldn't we be able to merge all command line switches (--browsers, IDEs, etc)?

webpack.config.js
@@ -18,6 +18,7 @@ module.exports = {
path: path.join(__dirname, '/dist'),
},
module: {
noParse: /\/src\/nativeRequire.js$/,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, lol

@ryhinchey ryhinchey force-pushed the envinfo-config-file branch from 4f16c2a to 40c1b39 Compare July 2, 2020 13:06
@tabrindle
Copy link
Owner

Are we reasonably confident with the state of this PR? I think it's good to go. We can iterate as we get adoption.

@ryhinchey
Copy link
Collaborator Author

Are we reasonably confident with the state of this PR? I think it's good to go. We can iterate as we get adoption.

@tabrindle yah I think so. I've been pretty busy lately but I did change the name of the file to envinfo.config.js. I'd like to land this with some docs which I can get to this week.

@eps1lon
Copy link

eps1lon commented Dec 7, 2020

How would the API for libraries look like? It seems to me that this PR would add support for an envinfo.config.js in the user repository. But that wouldn't be helpful since the library wants to control what environment information it needs.

I was about to file PRs for testing-library and Material-UI presets since I find this package incredibly helpful. Though I'm not sure if PRs for presets should be preferred or waiting for this feature.

@ryhinchey
Copy link
Collaborator Author

ryhinchey commented Feb 12, 2021 •
edited
Loading

How would the API for libraries look like? It seems to me that this PR would add support for an envinfo.config.js in the user repository. But that wouldn't be helpful since the library wants to control what environment information it needs.

I was about to file PRs for testing-library and Material-UI presets since I find this package incredibly helpful. Though I'm not sure if PRs for presets should be preferred or waiting for this feature.

sorry for the delay here. The goal of this PR is to come up with a way to avoid teams having to submit PRs for presets.

Maybe we should support being able to pass a --config flag to the envinfo command. Something like npx envinfo --config @testing-library/react that would tell envinfo to look for a config at the root of the @testing-library/react package within node_modules

@tabrindle thoughts on this approach?

@ryhinchey
Copy link
Collaborator Author

@tabrindle thoughts on #176 (comment)?

@tabrindle
Copy link
Owner

Yes, I think this approach would work. Would this need to be a separate feature/PR? I think this one is mostly complete for the original intentions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tabrindle tabrindle tabrindle requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ryhinchey @tabrindle @eps1lon