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(flow-react): avoid Flow-React portal outlet removal conflicts #20770

Merged
merged 17 commits into from
Jan 16, 2025

Conversation

platosha
Copy link
Contributor

Some routing cases in hybrid Flow layout + React view applications could produce DOM tree conflicts from Flow server-side changes and React client-side portal removal happening simultaneously. This could throw DOM NotFoundError in the browser. This change introduces a dedicated DOM element for React portal outlet, which allows to avoid the error.

Fixes vaadin/hilla#3002

Some routing cases in hybrid Flow layout + React view applications could produce DOM tree conflicts from Flow server-side changes and React client-side portal removal happening simultaneously. This could throw DOM `NotFoundError` in the browser. This change introduces a dedicated DOM element for React portal outlet, which allows to avoid the error.

Fixes vaadin/hilla#3002
Copy link

github-actions bot commented Dec 20, 2024

Test Results

1 163 files  ± 0  1 163 suites  ±0   1h 28m 2s ⏱️ - 3m 27s
7 620 tests ± 0  7 564 ✅ +15  56 💤 ±0  0 ❌  - 2 
7 970 runs  +14  7 905 ✅ +29  65 💤 ±0  0 ❌  - 2 

Results for commit 7c9a878. ± Comparison against base commit 3fe8592.

♻️ This comment has been updated with latest results.

@platosha platosha marked this pull request as ready for review December 20, 2024 15:46
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Still doesn't happen when manually running, but if I have the server open and run the hillaViewInFlowLayout test from vaadin-platform-react-hybrid-test it always fails with

Unexpected Application Error!
Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
NotFoundError: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
    at removeChildFromContainer (http://localhost:8080/VAADIN/@fs/C:/Users/.../platform/vaadin-platform-react-hybrid-test/node_modules/.vite/deps/chunk-UJKGNB4G.js?v=3c974d7f:8509:23)
    ```

@vaadin-bot vaadin-bot added +1.0.0 and removed +0.0.1 labels Dec 23, 2024
@Lodin Lodin requested a review from caalador December 23, 2024 12:14
@vaadin-bot vaadin-bot added +0.0.1 and removed +1.0.0 labels Dec 23, 2024
@platosha platosha requested a review from caalador January 7, 2025 14:24
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Could we do a PR with only formatting and then rebase this on it so that the changes would be clearer to see.
Also what is the formatter used as I get different results than those here. I don't think flow has any defined javascript format and uses the idea default instead.

@caalador
Copy link
Contributor

caalador commented Jan 8, 2025

For me locally the tests still fail in the hybrid module when executed [ERROR] Tests run: 12, Failures: 2, Errors: 2, Skipped: 0 the issue seems to still be the same. Also happens when only running one of the tests.
Locally clicking I still can't reproduce the issue.

@tepi
Copy link
Contributor

tepi commented Jan 9, 2025

Also what is the formatter used as I get different results than those here. I don't think flow has any defined javascript format and uses the idea default instead.

I think prettier is used, we have some rules for that in Flow

@ZheSun88
Copy link
Contributor

ZheSun88 commented Jan 9, 2025

when this PR got merged... please let us know that.. because we need to remove the @ignore from platform tests

@vaadin-bot vaadin-bot added +1.0.0 and removed +0.0.1 labels Jan 14, 2025
@Lodin Lodin requested a review from caalador January 14, 2025 13:15
@vaadin-bot vaadin-bot added +0.0.1 and removed +1.0.0 labels Jan 14, 2025
@caalador
Copy link
Contributor

Now I'm getting the exception even just clicking around. Most often repeated by going to /hilla and then to /flow from where I click to /flow/hello-hilla, but also just starting from flow and going to hilla often throws.

@caalador caalador merged commit 66443d4 into main Jan 16, 2025
26 checks passed
@caalador caalador deleted the fix/hilla-3002 branch January 16, 2025 11:11
@vaadin-bot
Copy link
Collaborator

Hi @platosha and @caalador, when i performed cherry-pick to this commit to 24.6, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 66443d4
error: could not apply 66443d4... fix(flow-react): avoid Flow-React portal outlet removal conflicts (#20770)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

Lodin pushed a commit that referenced this pull request Jan 16, 2025
…0770)

Some routing cases in hybrid Flow layout + React view applications could produce DOM tree conflicts from Flow server-side changes and React client-side portal removal happening simultaneously. This could throw DOM `NotFoundError` in the browser. This change introduces a dedicated DOM element for React portal outlet, which allows to avoid the error.

Fixes vaadin/hilla#3002

---------

Co-authored-by: Vlad Rindevich <vladrin@vaadin.com>

(cherry picked from commit 66443d4)
caalador pushed a commit that referenced this pull request Jan 16, 2025
…0770) (#20862)

Some routing cases in hybrid Flow layout + React view applications could produce DOM tree conflicts from Flow server-side changes and React client-side portal removal happening simultaneously. This could throw DOM `NotFoundError` in the browser. This change introduces a dedicated DOM element for React portal outlet, which allows to avoid the error.

Fixes vaadin/hilla#3002

---------

Co-authored-by: Vlad Rindevich <vladrin@vaadin.com>

(cherry picked from commit 66443d4)

Co-authored-by: Anton Platonov <anton@vaadin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigating between Views cause application Error
6 participants