Skip to content

Conversation

mhartington
Copy link
Contributor

Don't query for hidden inputs when using HMR

Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

I'd love to understand better what is happening. I tried to take a look at the reproduction https://github.com/mhartington/hrm-ionic-test but it's empty.

Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

Approved by mistake.

@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 16, 2020
@mhartington
Copy link
Contributor Author

@alan-agius4 pushed the code to hmr-ionic-test

Basically what's happening is that you have the following DOM structure

Screen Shot 2020-11-16 at 9 20 40 AM

Where ionic components like Range and Checkbox will render a hidden input within the component. So at the time HMR grabs a snapshot of the outgoing DOM structure, it will grab those hidden inputs. which do nothing and are not something users can directly interact with

So as HMR grabs the snapshot, it could count 1 actual input, and one hidden input. But the hidden input within Ionic's process might not be available right away when the changes are applied, meaning there will be a mismatch in the number of inputs.

Screen Shot 2020-11-16 at 9 26 21 AM

When hmr-accepts attempts to restore the values, with different input lengths it will noop out of the whole thing. Which in most cases, makes sense. But in this case, since the inputs are purposefully hidden and are not something that users can interact with, it's safe to ignore them.

You can see the dom changes yourself by setting a break point at restoreFormValues and inspecting the DOM

Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

@mhartington, thanks for the detailed explanation and the PR. The change LGTM.

Can you please squash the commits to make CI green?

Thanks

Closes angular#19385
Don't query for hidden inputs when using HMR
@mhartington mhartington force-pushed the fix-hmr-hidden-inputs branch from a3c91e5 to e7a8ea6 Compare November 16, 2020 15:25
@mhartington
Copy link
Contributor Author

Done!

@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 16, 2020
@clydin clydin merged commit 0f72ca4 into angular:master Nov 16, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants