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

Add separate file to configure this plugin #20

Closed
kriansa opened this issue Jan 18, 2018 · 9 comments
Closed

Add separate file to configure this plugin #20

kriansa opened this issue Jan 18, 2018 · 9 comments
Assignees

Comments

@kriansa
Copy link

kriansa commented Jan 18, 2018

Hello @Hargne

First of all, thanks for this plugin. It's been helpful to me 👍

I would like to suggest a feature request that allow the configuration to be set in a different file than package.json. It doesn't have to be necessarily the Jest main config, as suggested in #12, but keeping it separate makes sense in the perspective of separation of concerns.

Unfortunately, package .json has become very bloated, where every JS plugin just "injects" its configuration into it, but this pattern only increases the size of the file while keeps it unmaintainable over time, particularly because you can't just split it.

What do you think?

@pascalduez
Copy link
Contributor

pascalduez commented Jan 22, 2018

Hi,

several Jest related packages also use environment variables as an alternative to package.json keys.
See https://github.com/palmerj3/jest-junit#configuration

EDIT: just stumbled upon https://github.com/Hargne/jest-html-reporter#continuous-integration

@Hargne
Copy link
Owner

Hargne commented Jan 22, 2018

Hi @kriansa
Great feedback!

As @pascalduez mentioned, a few configurations can be made using environment variables: TEST_REPORT_PATH, TEST_REPORT_TITLE. However not all configuration options are available (which is actually something I should look into completing).

Personally i am not opposed to having multiple optional ways of configuring the plugin, so I will have a look at the best way to approach this.

@Hargne Hargne self-assigned this Jan 22, 2018
@Hargne Hargne added this to To do in New Features Jan 22, 2018
@pascalduez
Copy link
Contributor

Rather than creating yet another config file, would it be possible to extend and then read the jest config?
Just an idea.

@Hargne
Copy link
Owner

Hargne commented Jan 23, 2018

Additional environment variables have been added in v0.6.0 (#22) that offers an alternative way to configure this plugin (following the design of jest-junit, as suggested by @pascalduez in this comment).

No option for an extra configuration file has been added.
I believe that adding yet another config file with the sole purpose of configuring this plugin would both be redundant and confusing as plugins are normally configured within package.json.

Adding configuration to jest.config.js is not something that this plugin should do (please read this issue for an explanation).

Please re-open this issue if you don't agree with this :)

@Hargne Hargne closed this as completed Jan 23, 2018
@Hargne Hargne moved this from To do to Released in New Features Jan 23, 2018
@kriansa
Copy link
Author

kriansa commented Jan 23, 2018

Hey @Hargne ,

It's good to add environment variables so we can override settings at runtime, this is particularly useful for CI jobs. Thanks for doing that 🏆 👍

However, I think that the whole point of this issue is to provide a separate file so we avoid this pattern of adding every JS plugin config to package.json. I'll try to enumerate some points why I'm not comfortable with a library only providing support to configuring it through it:

  1. Semantically speaking, package.json was designed to describe a package. It's supposed to declare name, URL, dependencies and every information that is inherent to the package, not its setup. A dependency setup is not something that should not belong there.
    To add to that, using package.json is not natural. You won't find any JS API to help you managint it, and you need to parse the JSON as if it were just another file.
  2. package.json is a JSON, and it can't support complex structures or programming inside it. Due to that, you're limited to:
  • Not being able to split it into several other files
  • Not being able to add comments to help you 💬
  1. You compete with other plugins for namespacing and hope that no other plugin developer has an entry that may overlap yours.
  2. You're working with a pattern that you don't have control over. What if NPM decides to adopt another pattern for describing package dependency? What if another package management tool is created and it decides to use something else as a package description?
  3. You're breaking a pattern. 📦 NPM describes package.json here, and it declares all the keys that should/could be present. Once a plugin starts using it as its own, it starts breaking this pattern. What if NPM decides to use a key that a plugin is using?

Many other plugins also use that feature, so the developer tends to think that it's normal, as you said yourself. However, this line of thought is turning package.json big and messy and difficult to manage, where you can find just about anything,

That being said, let me say: I don't want to propose dropping support to configure the plugin using package.json. Rather, I want to give people options so that they can choose as they want.

Also, maybe injecting into jest.config.js is just another example of what I just described. Instead, why not a separate file? We could go on discussing the pros/cons of each alternative.

I just wanted to bring up this discussion before submitting any patch. What do you guys think?

@Hargne
Copy link
Owner

Hargne commented Jan 23, 2018

Hi @kriansa
Thanks for the detailed response!

As I said previously, I am not personally opposed to having multiple ways of configuring a plugin. But then I was also not entirely convinced of having an extra file containing just 1-4 rows of code, seeing that this is such a small plugin (and also the fact that we would eventually end up with loads of config files bloating our folders instead).

However, after hearing your point of view, I must admit that I agree to the fact that it should at least be a possibility to use such a file if that is something that would benefit your project.

If you have a suggested fix for it, please do submit a pull request and I will gladly have a look at it! :)

@Hargne Hargne reopened this Jan 23, 2018
@kriansa
Copy link
Author

kriansa commented Jan 23, 2018

You're right, a file might sound like an overkill for such a small plugin. Ideally, the right thing IMO would be jest support passing a config object to the plugins as well.

I will work on that this weekend and submit a patch soon. Thank you for being open to the discussion. 😄

@Hargne Hargne moved this from Released to In progress in New Features Jan 24, 2018
Hargne pushed a commit that referenced this issue Feb 1, 2018
…nfig.json, Removed deprecated environment variable configurations, Converted style into CSS, Added new styling, Added build scripts with RollupJS, Added ESLint, Updated documentation
Hargne pushed a commit that referenced this issue Feb 1, 2018
… future themes, Moved the default style into /style, Updated documentation
Hargne added a commit that referenced this issue Feb 1, 2018
@Hargne
Copy link
Owner

Hargne commented Feb 1, 2018

Hi @kriansa

I went ahead and implemented configuration via a standalone config file along with some other new features and improvements. All this is available in v1.0.1.

I'm sorry if I took the task away from you, but I realised that there were some general optimisations and improvements I wanted to get out as well :)

Please return ASAP and notify me if we can close this issue.

@kriansa
Copy link
Author

kriansa commented Feb 2, 2018

Wow!! Awesome @Hargne !!

Thank you for working on that, I loved the new features!

@kriansa kriansa closed this as completed Feb 2, 2018
@Hargne Hargne moved this from In progress to Released in New Features Feb 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
New Features
  
Released
Development

No branches or pull requests

3 participants