Conversation
|
@fanshi1028 Thanks for submitting this. Did you run through all the examples to test? I'm getting a couple Typescript errors on your branch. Can you fix please? Do we have a typescript version mismatch or somethign? |
Oops. I just tsc-ed it and didn't even try to build with webpack. But I have just fixed them for webpack build and checked to ensured the demo app's examples do works. Please check again. |
| export function listenBubblingEvent(event: string, cb: (_target: HyperView, action: string) => void): void { | ||
| document.addEventListener(event, function(e) { | ||
| let el = e.target as HTMLInputElement | ||
| if (!(e.target instanceof HTMLElement)) { |
There was a problem hiding this comment.
Is this necessary? HyperView extends HTMLElement. Perhaps this belongs at the callsite (or perhaps it already has a check there).
Either way, we shouldn't return without a console error
There was a problem hiding this comment.
Theoretically some third party code could do thing like this:
document.addEventListener("click", function(e){
if (e.target instanceof HTMLElement) {
console.log(e)
} else console.error("event target is not a HTMLElement %o", e)
})
document.dispatchEvent(new Event("click"))
The event's target will be null and break the original code.
No checking on the call site of listenBubblingEvent could ever prevent this.
In practice though, maybe no one will ever do this.
There was a problem hiding this comment.
I think if someone really dispatch their own event like this, whatever spaghetti they are into, it is their own business. Then when our handlers eavesdrop and catch them, this doesn't look like an "error" to me, so I just chose to ignored them in the code. But I think we can also warn them if you think we should, so I pushed a commit to "console.warn" about them now.
There was a problem hiding this comment.
Tell me what you think. If you still think these checks are unnecessary(as it is really kind of a niche edge case). We could revert them all back to simple as HTMLElement.
client/src/events.ts
Outdated
| let onMouseEnter = node.dataset.onmouseenter | ||
|
|
||
| let target = nearestTarget(node) | ||
| let target = nearestNonHyperViewTarget(node) |
There was a problem hiding this comment.
This could theoretically still be a hyper view? I think we mean that it isn't required to be a HyperView.
Perhaps rename nearestTarget to nearestHyperViewTarget and rename this to nearestAnyTarget?
| export function listenChange(cb: (target: HyperView, action: string) => void): void { | ||
| document.addEventListener("change", function(e) { | ||
| let el = e.target as HTMLElement | ||
| if (!(e.target instanceof HTMLElement)) { |
| document.addEventListener("input", function(e) { | ||
| let el = e.target as HTMLElement | ||
| let source = el.closest("[data-oninput]") as LiveInputElement | ||
| if (!(e.target instanceof HTMLElement)) { |
| if (!source) return | ||
|
|
||
| let delay = parseInt(source.dataset.delay) || 250 | ||
| let delay = parseInt(source.dataset.delay || "") || 250 |
There was a problem hiding this comment.
Interesting, does parseInt(x) || 250 fail on undefined?
There was a problem hiding this comment.
Never. But we needs this unnecessary effort to please the compiler. parseInt take a string there.
client/src/events.ts
Outdated
| console.error("Missing target: listenChange") | ||
| return | ||
| } | ||
| if (source.dataset.onchange === undefined) { |
There was a problem hiding this comment.
Simplify to if (!source.dataset.onchange) like you did for oninput
| export function listenFormSubmit(cb: (target: HyperView, action: string, form: FormData) => void): void { | ||
| document.addEventListener("submit", function(e) { | ||
| let form = e.target as HTMLFormElement | ||
| if (!(e.target instanceof HTMLFormElement)) { |
client/src/events.ts
Outdated
| } | ||
|
|
||
| function nearestTarget(node: HTMLElement): HTMLElement { | ||
| function nearestTarget(node: HTMLElement): HyperView | undefined { |
There was a problem hiding this comment.
See notes above about renaming this and nearestNonHyperViewTarget
| @@ -0,0 +1,15 @@ | |||
| import { type Request } from "./action"; | |||
There was a problem hiding this comment.
Let's move enrichHyperViews to this module? Plus anything else that's directly related to this interface.
There was a problem hiding this comment.
if we do that. We are moving almost all code in index.ts into it. As the code and quite tightly tangled together. enrichHyperViews need runAction. and runAction need the sock and its initialisation logic.
Here are some ways I imagine we can do about it.
- Just move it all. (Moving those initialisation code from index.ts into it doesn't feel right to me, but you tell me).
- Just move enrichHyperViews and make it take and an extra param "runAction".
- move the hyperView.ts code into event.ts (as only index.ts and event.ts import it)
- Don't move it and keep it as is.
- (Tell me if you have any other prefered solution)
I have chosen to do 2. Tell me it you prefer to do it other way.
|
|
||
| // Should we connect to the socket or not? | ||
| const sock = new SocketConnection() | ||
| sock.connect() |
There was a problem hiding this comment.
Where do we connect the socket? Does it happen automatically?
There was a problem hiding this comment.
I somehow moved it into the constructor, but now that I think about it, yup, I moved it back out.
… to nearestAnyTarget
Changed logging level from error to warn when event handlers receive non-HTMLElement targets, and standardized log messages using %o formatter for better object inspection.
This reverts commit 51cb0bb.
|
@seanhess Please check my replies and changes and tell me what to do next(if any). |
No description provided.