Skip to content

Conversation

@P0lip
Copy link
Contributor

@P0lip P0lip commented Oct 15, 2019

Fixes #58
A note on snapshot change - it shouldn't have any impact, since the actual node that gets rendered in tree-list has a proper type assigned - see line 146 and line 197.
It's just that the other nodes point at original object.

You can also verify it manually in storybook by pasting the schema file and comparing the view. :P

This got me realize we actually seem to squeeze too much into metadata. It seems like we could make it much leaner.

@P0lip P0lip added the t/bug Something isn't working label Oct 15, 2019
@P0lip P0lip requested a review from marbemac October 15, 2019 22:54
@P0lip P0lip self-assigned this Oct 15, 2019
for (const [i, property] of node.properties.entries()) {
if ('type' in node) {
property.type = property.type || node.type;
node.properties[i] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

So is this needed in general? Feels like it might not be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's needed. If you revert it, a few tests covering mutation will fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure? For me only two tests fail, both snapshots regarding the "type": "object" thing. Not saying that means everything is good, but just that the tests don't seem to indicate any big issues?

Screen Shot 2019-10-15 at 6 58 00 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me only two tests fail,

Yup, these are the only ones, since they cover the combiner children case.

Not saying that means everything is good, but just that the tests don't seem to indicate any big issues?

That's right, seems like this was the only spot.

@P0lip P0lip merged commit 212f09b into master Dec 30, 2019
@P0lip P0lip deleted the fix/do-not-mutate-type branch December 30, 2019 18:46
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version 2.11.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released t/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JSON schema viewer should not mutate

4 participants