Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(package): update schema-utils v0.3.0...0.4.1 (dependencies) #668

Closed
wants to merge 2 commits into from

Conversation

dinkzilla
Copy link

@dinkzilla dinkzilla commented Nov 21, 2017

Issues

Notable Changes

This pull request properly catches and throws validation errors within schema-utils so that they appear appropriately in the console.

As an example, a validation error which used to read as follows

<project-path>\node_modules\schema-utils\dist\validateOptions.js:40
    throw new _ValidationError2.default(ajv.errors, name);
    ^

false

now reads e.g

Extract Text Plugin Invalid Options

options['sourceMap'] should NOT have additional properties

@jsf-clabot
Copy link

jsf-clabot commented Nov 21, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@joshwiens joshwiens left a comment

Choose a reason for hiding this comment

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

Test suite will need to be updated accordingly

@codecov
Copy link

codecov bot commented Nov 22, 2017

Codecov Report

Merging #668 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #668   +/-   ##
=======================================
  Coverage   87.45%   87.45%           
=======================================
  Files           7        7           
  Lines         303      303           
  Branches       68       68           
=======================================
  Hits          265      265           
  Misses         36       36           
  Partials        2        2

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 cc3ba94...7d97a94. Read the comment docs.

@dinkzilla
Copy link
Author

Based on conversation in the #667 thread with @michael-ciniawsky, The correct fix was actually to update to the newest version of schema-utils, which now logs errors to the console. I have updated this pull request accordingly. Schema-utils will now log the error to the console and then exit, which required updating the validation failure test.

@@ -15,9 +15,13 @@ describe('json schema validation', () => {
}).doesNotThrow;
});

it('throws if an incorrect config is passed in', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I'm sry about this 😅 I should have taken a look sooner but could you add process.env.JEST = true on L3-4 instead so schema-utils will rethrow the {Error} instead of exiting while running tests ? The 'old' test should work then.

const err = () => new ExtractTextPlugin({ filename: 1 })

expect(err).toThrow()
// Test if we get the correct err.message
expect(err).toThrowErrorMatchingSnapshot()

Copy link
Author

Choose a reason for hiding this comment

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

But in reality, doesn't it write to console and exit, not throw? My test was written to match that scenario.

The test you describe would essentially be "when in testing with a bad config, does it throw?" Unless I'm missing something, that doesn't seem like a very valuable test.

(Not sure why one of the checks is failing, I've been on holiday and but tests were passing on my computer before I left. Can look into this tomorrow, and will update based on your response.)

Copy link
Member

Choose a reason for hiding this comment

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

The test should be changed back, but please hold on for ~1-2 days we resolved this in webpack core (#6035) and schema-utils (#17)

Copy link
Author

Choose a reason for hiding this comment

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

@michael-ciniawsky Can you explain why the test should be changed back? I am still confused about what it is testing.

Copy link
Member

Choose a reason for hiding this comment

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

It's tests for malformed options based on a JSON Schema and should work without the need to mock anything in the test, please revert the test changes and I will finally land this on master :)

@michael-ciniawsky michael-ciniawsky added this to the 3.0.2 milestone Nov 22, 2017
@michael-ciniawsky michael-ciniawsky changed the title fix: Issue #667 - catch and display schema-utils validation errors fix(package): update schema-utils v0.3.0...0.4.1 Nov 22, 2017
@michael-ciniawsky michael-ciniawsky modified the milestones: 3.0.3, 3.1.0 Nov 29, 2017
@alexander-akait
Copy link
Member

@dinkzilla friendly ping 👍

@dinkzilla
Copy link
Author

You caught me, been busy and totally forgot this was out here. I'll try to get to it this weekend. Thanks for the ping @evilebottnawi

@@ -15,9 +15,13 @@ describe('json schema validation', () => {
}).doesNotThrow;
});

it('throws if an incorrect config is passed in', () => {
Copy link
Member

Choose a reason for hiding this comment

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

It's tests for malformed options based on a JSON Schema and should work without the need to mock anything in the test, please revert the test changes and I will finally land this on master :)

@michael-ciniawsky
Copy link
Member

I will update it myself with the next patch release

@michael-ciniawsky michael-ciniawsky changed the title fix(package): update schema-utils v0.3.0...0.4.1 fix(package): update schema-utils v0.3.0...0.4.1 (dependencies) Mar 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants