Skip to content
This repository was archived by the owner on Sep 28, 2020. It is now read-only.

a crack at solving #105 #123

Merged
merged 3 commits into from
Nov 2, 2016
Merged

a crack at solving #105 #123

merged 3 commits into from
Nov 2, 2016

Conversation

jameslnewell
Copy link
Contributor

No description provided.

@jameslnewell
Copy link
Contributor Author

Oh wait, I missed #106

@jameslnewell
Copy link
Contributor Author

Reopening because #106 is only creating a new engine when a configFile prop is used. The config can also be passed as an object.

@jaythomas
Copy link

I like your more-robust solution over #106. One thing you might want to adjust however is the test:

"should report a single error because only one config causes an error" does not infer that "eslint-loader will create an engine for each unique config". Whether or not the second config is ignored, it will always report only one warning (not error). In my test I assured both configs were loading by throwing a unique warning from each: https://github.com/MoOx/eslint-loader/pull/106/files#diff-d1a0eb3a6e5c2e85a9cd54e5f92bfd7fR85

Feel free to borrow my solution or disregard!

@jameslnewell
Copy link
Contributor Author

jameslnewell commented Oct 29, 2016

Thanks!

Sorry, I'm not sure if I follow what you're saying... With my test, in the case where:

  • the first eslint config is used for both instances, there will be 2 warnings generated (and we're expecting 1)
  • the second eslint config is used for both instances, there will be 0 warnings generated (and we're expecting 1)
  • no eslint configs are used, there will be 0 warnings generated (and we're expecting 1)
  • both eslint configs are used, only the first will trigger a warning (and we're expecting 1) so it will pass

@jaythomas
Copy link

What I mean to say is here, this rule will never throw an error so your assertion that stats.compilation.warnings.length === 1 is always true even if that second config object is never loaded or if there were another unforeseen problem with the webpack settings. So the test doesn't directly verify that "eslint-loader will create an engine for each unique config" as your test is named.

My suggestion around this would be to change the second config object rule from quotes: [1, "double"] to something that will throw a second warning, (semi: [1, "always"] for instance). You will know the second config is loading correctly if it throws the expected warning from the second config. You can then change the assertion to read stats.compilation.warnings.length === 2 instead of === 1.

@jameslnewell
Copy link
Contributor Author

Cool, I think I get you, however that introduces the problem where if only the first config is used, you'll still get two (quote) errors and the test will incorrectly pass. Do we need two separate tests? Got any ideas?

@jameslnewell
Copy link
Contributor Author

^^checking that I get two different types of warnings

@jaythomas
Copy link

I have closed PR #106 in favor of this one.

@jameslnewell
Copy link
Contributor Author

@MoOx thoughts?

@MoOx MoOx merged commit 672a100 into webpack-contrib:master Nov 2, 2016
@MoOx
Copy link
Contributor

MoOx commented Nov 2, 2016

Great work!

@MoOx
Copy link
Contributor

MoOx commented Nov 2, 2016

Releasing this as 1.6.1. Thank you @jameslnewell and @jaythomas!

deryni added a commit to deryni/eslint-loader that referenced this pull request Dec 15, 2016
* MoOx/master:
  README: Add information about eslint behaviour when configFile is set directly (webpack-contrib#129) (webpack-contrib#130)
  Add clarifying note to README (webpack-contrib#127)
  Removed test.only so that all tests will run (webpack-contrib#125)
  1.6.1
  Fixed: multiples config per instance are now supported (webpack-contrib#123)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants