Fix: Browser tab freezes, unresponsive, plus possible memory leak with form in modal (#4018) #4021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Fixes the bug that `Field` caused infinite recursions when both `grouped` and `horizontal` attributes were `true`, and a validation error was set. There were circular updates of the `newMessage` data field and the `message` prop of the child `Field`s. To break the loop, `watch` of the `message` prop makes sure that the contents of the new value are different from the `newMessage` data field before updating `newMessage`. Simple identity check won't work because `message` receives a brand new array every time.
- Adds new test cases that test `Field` with `grouped` and `horizontal` attributes `true`. The test would never stop due to infinite recursions if the previous fix were not applied.
- Adds the fix of `Field` freezing due to infinite recursions.
- Adds warnings of not mixing `grouped` and `horizontal` props of `Field`.
✅ Deploy Preview for buefy-org ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
wesdevpro
requested changes
Mar 11, 2024
| @@ -69,7 +69,7 @@ export default [ | |||
| }, | |||
| { | |||
| name: '<code>horizontal</code>', | |||
| description: 'Group label and control on the same line for horizontal forms', | |||
| description: 'Group label and control on the same line for horizontal forms. Do not mix with <code>grouped</code>.', | |||
There was a problem hiding this comment.
Is there a more clear way to say this? And should we also explain, in a simple form, why we shouldn't mix them?
There was a problem hiding this comment.
I briefly explained what will happen if we combine them. However, there would not be a reasonable explanation because it is a design flaw we have to address.
There was a problem hiding this comment.
DM over discord. Maybe we could discuss this flaw over the weekend?
- Better explains the reason why `group` and `horizontal` props of `Field` should not be mixed.
- Explains the issue that remains after fixing the frozen Field bug.
wesdevpro
approved these changes
Mar 12, 2024
wesdevpro
approved these changes
Mar 12, 2024
7 tasks
kikuomax
added a commit
to kikuomax/buefy
that referenced
this pull request
Apr 2, 2024
…h form in modal (buefy#4018) (buefy#4021) * fix(lib): infinite Field updates - Fixes the bug that `Field` caused infinite recursions when both `grouped` and `horizontal` attributes were `true`, and a validation error was set. There were circular updates of the `newMessage` data field and the `message` prop of the child `Field`s. To break the loop, `watch` of the `message` prop makes sure that the contents of the new value are different from the `newMessage` data field before updating `newMessage`. Simple identity check won't work because `message` receives a brand new array every time. * test(lib): add test cases for Field with grouped and horizontal - Adds new test cases that test `Field` with `grouped` and `horizontal` attributes `true`. The test would never stop due to infinite recursions if the previous fix were not applied. Includes adaptation for @vue/test-utils V2: - `find(Component)` → `findComponent(Component)` - `findAll(Component)` → `findAllComponents(Component)` - `.at(i)` → `[i]` when accessing an item in `findAllComponents` results. @vue/test-utils V2 returns an ordinary array - `localVue` → `global: { components }`: @vue/test-utils V2 no longer supports `localVue` - `propsData` → `props`: `propsData` still works, but prepares for possible future deprecation * docs(CHANGELOG): update unreleased log - Adds the fix of `Field` freezing due to infinite recursions. - Explains the issue that remains after fixing the frozen Field bug. * chore(docs): add warning on grouped and horizontal (Field) - Adds warnings of not mixing `grouped` and `horizontal` props of `Field`.
kikuomax
added a commit
to ntohq/buefy-next
that referenced
this pull request
Apr 3, 2024
…h form in modal (buefy#4018) (buefy#4021) * fix(lib): infinite Field updates - Fixes the bug that `Field` caused infinite recursions when both `grouped` and `horizontal` attributes were `true`, and a validation error was set. There were circular updates of the `newMessage` data field and the `message` prop of the child `Field`s. To break the loop, `watch` of the `message` prop makes sure that the contents of the new value are different from the `newMessage` data field before updating `newMessage`. Simple identity check won't work because `message` receives a brand new array every time. * test(lib): add test cases for Field with grouped and horizontal - Adds new test cases that test `Field` with `grouped` and `horizontal` attributes `true`. The test would never stop due to infinite recursions if the previous fix were not applied. Includes adaptation for @vue/test-utils V2: - `find(Component)` → `findComponent(Component)` - `findAll(Component)` → `findAllComponents(Component)` - `.at(i)` → `[i]` when accessing an item in `findAllComponents` results. @vue/test-utils V2 returns an ordinary array - `localVue` → `global: { components }`: @vue/test-utils V2 no longer supports `localVue` - `propsData` → `props`: `propsData` still works, but prepares for possible future deprecation * docs(CHANGELOG): update unreleased log - Adds the fix of `Field` freezing due to infinite recursions. - Explains the issue that remains after fixing the frozen Field bug. * chore(docs): add warning on grouped and horizontal (Field) - Adds warnings of not mixing `grouped` and `horizontal` props of `Field`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #
Proposed Changes
Fieldmakes sure that the contents of a new value fornewMessageare different from the currentnewMessagebefore updating it to avoid infinite recursions that made a browser tab freezegroupedandhorizontalprops ofFieldto the documentationRemarks
Since
Fieldwas not designed to turn ongroupedandhorizontalprops simultaneously, another issue will emerge after merging this PR, where once a validation error state is set, the error state won't be reset even after correcting the input. A workaround for now is not to mixgroupedandhorizontal. This PR will mitigate the worst scenario anyway.