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

fix(core): update nimma & json-schema #2012

Merged
merged 2 commits into from
Dec 29, 2021
Merged

fix(core): update nimma & json-schema #2012

merged 2 commits into from
Dec 29, 2021

Conversation

P0lip
Copy link
Contributor

@P0lip P0lip commented Dec 28, 2021

Fixes #1999

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Additional context

List of notable changes

@P0lip P0lip changed the title fix(core): update nimma fix(core): update nimma & json-schema Dec 28, 2021
Comment on lines +41 to +54
} catch (e: unknown) {
if (
e instanceof Error &&
Array.isArray((e as Error & { errors?: unknown }).errors) &&
(e as Error & { errors: unknown[] }).errors.length === 1
) {
const actualError = (e as Error & { errors: [unknown] }).errors[0];
throw actualError instanceof Error && 'cause' in (actualError as Error & { cause?: unknown })
? (actualError as Error & { cause: unknown }).cause
: actualError;
} else {
throw e;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hopefully this can be simplified once AggregateError and error causes are somehow exposed in standard lib

Copy link
Contributor

Choose a reason for hiding this comment

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

So what's going on here? Don't quite understand this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's to simplify testing.
Assertion of AggregateError is shallow, meaning nested errors are not checked.
This change makes it so we re-throw a single error so toThrow works as expected.
In 99% (if not 100%) of cases we have a single error thrown, so that's why we unpack that AggregateError.
Makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously we threw a single exception when AggregateErrors.errors.length === 1, but I simplified it in the code.
This, however, meant we needed to move that logic in here, but it's a test helper.

Comment on lines -118 to -124
} catch (e) {
if (isAggregateError(e) && e.errors.length === 1) {
throw e.errors[0];
} else {
throw e;
}
}
Copy link
Contributor Author

@P0lip P0lip Dec 28, 2021

Choose a reason for hiding this comment

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

@P0lip P0lip marked this pull request as ready for review December 28, 2021 19:40
@P0lip P0lip self-assigned this Dec 28, 2021
@P0lip P0lip added the dependencies Pull requests that update a dependency file label Dec 28, 2021
@P0lip P0lip requested a review from a team December 28, 2021 19:40
@P0lip P0lip enabled auto-merge (squash) December 28, 2021 19:46
Copy link
Contributor

@marbemac marbemac left a comment

Choose a reason for hiding this comment

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

Have a Q re the change in the test file - otherwise straight forward enough.

Comment on lines +41 to +54
} catch (e: unknown) {
if (
e instanceof Error &&
Array.isArray((e as Error & { errors?: unknown }).errors) &&
(e as Error & { errors: unknown[] }).errors.length === 1
) {
const actualError = (e as Error & { errors: [unknown] }).errors[0];
throw actualError instanceof Error && 'cause' in (actualError as Error & { cause?: unknown })
? (actualError as Error & { cause: unknown }).cause
: actualError;
} else {
throw e;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So what's going on here? Don't quite understand this change.

@P0lip P0lip merged commit 67a6104 into develop Dec 29, 2021
@P0lip P0lip deleted the fix/core/nimma-bump branch December 29, 2021 04:22
stoplight-bot pushed a commit that referenced this pull request Dec 29, 2021
# [@stoplight/spectral-core-v1.8.1](https://github.com/stoplightio/spectral/compare/@stoplight/spectral-core-v1.8.0...@stoplight/spectral-core-v1.8.1) (2021-12-29)

### Bug Fixes

* **core:** update nimma & json-schema ([#2012](#2012)) ([67a6104](67a6104))
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version @stoplight/spectral-core-v1.8.1 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

stoplight-bot pushed a commit that referenced this pull request Dec 29, 2021
# [@stoplight/spectral-functions-v1.5.1](https://github.com/stoplightio/spectral/compare/@stoplight/spectral-functions-v1.5.0...@stoplight/spectral-functions-v1.5.1) (2021-12-29)

### Bug Fixes

* **core:** update nimma & json-schema ([#2012](#2012)) ([67a6104](67a6104))
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version @stoplight/spectral-functions-v1.5.1 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update json-schema dependency in core
3 participants