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: add ValidationError #8

Merged
merged 8 commits into from
Apr 29, 2017
Merged

feat: add ValidationError #8

merged 8 commits into from
Apr 29, 2017

Conversation

michael-ciniawsky
Copy link
Member

schemautilserror

@michael-ciniawsky
Copy link
Member Author

@bebraw @d3viant0ne please review

@codecov
Copy link

codecov bot commented Apr 20, 2017

Codecov Report

Merging #8 into master will increase coverage by 10.78%.
The diff coverage is 94.11%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master       #8       +/-   ##
===========================================
+ Coverage   83.33%   94.11%   +10.78%     
===========================================
  Files           2        3        +1     
  Lines           6       17       +11     
  Branches        2        3        +1     
===========================================
+ Hits            5       16       +11     
  Misses          1        1
Impacted Files Coverage Δ
src/cjs.js 0% <0%> (ø)
src/ValidationError.js 100% <100%> (ø)
src/validateOptions.js 100% <100%> (ø)

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 7152276...f40a7cd. Read the comment docs.

@michael-ciniawsky
Copy link
Member Author

Fixes #5 #6 #7

@@ -0,0 +1,26 @@
/* eslint-disable */
import validateOptions from '../dist/validateOptions';
Copy link
Member

Choose a reason for hiding this comment

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

You keep defaulting to this testing approach and as I have said before, you negate most of the usefulness of using Jest.

  • Jest doesn't require a build step as it's setup in defaults. babel-jest takes care of that.
  • Jest runs tests async & only runs the tests related to the code you have changed, this negates that as well.
  • None of the editor plugins for Jest work with this approach

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@michael-ciniawsky
Copy link
Member Author

Anything else outstanding here ? Otherwise please merge and release I need this to move on with webpack-defaults 😛

@bebraw
Copy link
Contributor

bebraw commented Apr 24, 2017

Yeah, I would merge this and get a new version out. Still a fresh package so better move fast.

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented Apr 24, 2017

It's MVP 😛 I intentionally avoided anything AJV 'advanced' for now and update schema-utils if we hit any limits with the current approach, but Load Schema => Validate Schema => Validate Options ? true : throw new ValidationError() works and should be fine for 90% of usecases in Loaders/Plugins, at least we can add it to all repos with the webpack-defaults upgrade and minor improvements follow :)

  • webpack-defaults release with latest updates
  • schema-utils release with this PR

and I'm rolling again 😛

⚠️ This is technically a breaking change, bc of ajv@5.0.0 (no $schema), but I'm aware of that especially for ETWP

@michael-ciniawsky
Copy link
Member Author

@bebraw ping 😛

@joshwiens
Copy link
Member

@michael-ciniawsky - Juho is still touring as far as I know so his availability is limited.

Given this is an internal tool and limited in deployment at the moment, as long as you are finished with this PR, I'll land it and publish a Major.

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented Apr 27, 2017

Juho is still touring as far as I know so his availability is limited

@d3viant0ne ohh.. sry I wasn't aware of that :)
Thx 👍 the PR is finished and additionally tested locally with a dummy loader/plugin && webpack

validationerror

Loader === ${Name} in the screenshot 😛

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented Apr 27, 2017

@d3viant0ne Maybe this error format is better

ERROR in 'bla.js'
Module build failed: Validation Error
${Name} invalid options found: 

options.foo should be bar
ERROR in 'bla.js'
Module build failed:
VALIDATION ERROR in ${Name} 
Invalid options found: 

options.foo should be bar

?

Copy link
Contributor

@bebraw bebraw left a comment

Choose a reason for hiding this comment

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

Let's go with this. The exact formatting can be tweaked later based on user feedback.

@michael-ciniawsky
Copy link
Member Author

:shipit: 😛

@joshwiens joshwiens merged commit d48f0fb into webpack:master Apr 29, 2017
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.

None yet

3 participants