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 minimize #10952

Closed
wants to merge 13 commits into from
Closed

Fix minimize #10952

wants to merge 13 commits into from

Conversation

IslandRhythms
Copy link
Collaborator

No description provided.

@IslandRhythms IslandRhythms self-assigned this Nov 3, 2021
@IslandRhythms
Copy link
Collaborator Author

The failing linter test is weird because that line that it is referencing has no code on it.

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Don't worry about the lint failure, I'll fix that in master

if (options._calledWithOptions.minimize != null) {
_minimize = options.minimize;
} else if (defaultOptions.minimize != null) {
_minimize = defaultOptions.minimize;
} else if (options._parentOptions != null && options.getters != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why options.getters != null here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it won't work without it. gh-9405 has tests already and this was the only combination that could get them all to pass.

lib/document.js Outdated Show resolved Hide resolved
@vkarpov15
Copy link
Collaborator

I took a closer look and this is expected behavior. We made the change in #9405 that subdocs inherit parents' minimize and added tests that enforce that behavior, so no way to make this work. The getters trick works by accident rather than by design.

If you find yourself struggling to reconcile behavior that's asserted on in the tests with new behavior, feel free to ask me to clarify in Slack.

@vkarpov15 vkarpov15 closed this Nov 10, 2021
@Uzlopak Uzlopak deleted the fix-minimize branch June 15, 2022 11:14
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.

Missing breaking change in v6 changelog: schema no longer inherits minimize option
2 participants