-
Notifications
You must be signed in to change notification settings - Fork 785
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(renderer): fix conditional rendering issue #5365
fix(renderer): fix conditional rendering issue #5365
Conversation
fix conditional rendering issue by applying to find actual DOM element(s) when about the relocate element is slot ref fixes one of the issues raised in ionic-team#5335
|
Path | Error Count |
---|---|
src/dev-server/index.ts | 37 |
src/dev-server/server-process.ts | 32 |
src/compiler/prerender/prerender-main.ts | 22 |
src/testing/puppeteer/puppeteer-element.ts | 22 |
src/runtime/client-hydrate.ts | 20 |
src/screenshot/connector-base.ts | 19 |
src/runtime/vdom/vdom-render.ts | 17 |
src/dev-server/request-handler.ts | 15 |
src/compiler/prerender/prerender-optimize.ts | 14 |
src/compiler/sys/stencil-sys.ts | 14 |
src/compiler/transpile/transpile-module.ts | 14 |
src/sys/node/node-sys.ts | 14 |
src/compiler/prerender/prerender-queue.ts | 13 |
src/compiler/sys/in-memory-fs.ts | 13 |
src/runtime/connected-callback.ts | 13 |
src/runtime/set-value.ts | 13 |
src/compiler/output-targets/output-www.ts | 12 |
src/compiler/transformers/test/parse-vdom.spec.ts | 12 |
src/compiler/transformers/transform-utils.ts | 12 |
src/mock-doc/test/attribute.spec.ts | 12 |
Our most common errors
Typescript Error Code | Count |
---|---|
TS2322 | 361 |
TS2345 | 349 |
TS18048 | 206 |
TS18047 | 82 |
TS2722 | 37 |
TS2532 | 24 |
TS2531 | 21 |
TS2454 | 14 |
TS2790 | 11 |
TS2352 | 10 |
TS2769 | 8 |
TS2538 | 8 |
TS2416 | 6 |
TS2493 | 3 |
TS18046 | 2 |
TS2684 | 1 |
TS2430 | 1 |
Unused exports report
There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!
Unused exports
File | Line | Identifier |
---|---|---|
src/runtime/bootstrap-lazy.ts | 21 | setNonce |
src/screenshot/screenshot-fs.ts | 18 | readScreenshotData |
src/testing/testing-utils.ts | 198 | withSilentWarn |
src/utils/index.ts | 145 | CUSTOM |
src/utils/index.ts | 269 | normalize |
src/utils/index.ts | 7 | escapeRegExpSpecialCharacters |
src/compiler/app-core/app-data.ts | 25 | BUILD |
src/compiler/app-core/app-data.ts | 115 | Env |
src/compiler/app-core/app-data.ts | 117 | NAMESPACE |
src/compiler/fs-watch/fs-watch-rebuild.ts | 123 | updateCacheFromRebuild |
src/compiler/types/validate-primary-package-output-target.ts | 61 | satisfies |
src/compiler/types/validate-primary-package-output-target.ts | 61 | Record |
src/testing/puppeteer/puppeteer-declarations.ts | 485 | WaitForEventOptions |
src/compiler/sys/fetch/write-fetch-success.ts | 7 | writeFetchSuccessSync |
PR built and packed!Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8193146849/artifacts/1306949133 If your browser saves files to
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for raising a PR @yigityuce 🙏
Can I ask you do to add an end-to-end test for this to ensure we don't regress on this? I recommend to look into test/karma/test-app
and add a new slot-XXX folder and copy your example from the reproduction case.
Let me know if you have any questions or if I can assist you.
…conditional-slot-rendering
f9410c3
to
f1f3a28
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect 👌 this LGTM 👍
Let's have someone else from the @ionic-team/stencil review this.
hey @christian-bromann & @rwaskiewicz do I need to do sth else to move forward? |
@yigityuce Nope 🙂 I want to land some other infrastructure work to test this out a little more with the Ionic Framework before we land this, but plan on taking a look at the PR itself later today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thanks for your patience!
I need to create a PR for the Stencil Site for this PR, I'll merge this once the docs-PR is approved |
hey @rwaskiewicz thanks for the update! If I can do anything to help for that docs side, please let me know |
this pr accompanies ionic-team/stencil#5365, which is the fix for https://github.com/ionic-team/stencil/issue/5335
…#1374) this pr accompanies ionic-team/stencil#5365, which is the fix for https://github.com/ionic-team/stencil/issue/5335
This has been released with Stencil 🚞 v4.13.0 |
fix conditional rendering issue by applying to find actual DOM element(s) when about the relocate element is slot ref
fixes one of the issues raised in #5335
What is the current behavior?
Slot elements are not rendered conditionally if they are not wrapped with builtin element such as div.
GitHub Issue Number: #5335
What is the new behavior?
Fixed
Documentation
Does this introduce a breaking change?
Testing
Other information