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

Event + CustomEvent in all environments #46

Closed
ryhinchey opened this issue Apr 22, 2020 · 10 comments
Closed

Event + CustomEvent in all environments #46

ryhinchey opened this issue Apr 22, 2020 · 10 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@ryhinchey
Copy link
Contributor

Currently new CustomEvent will only work in the browser. I see a need to have an implementation that works in node as well. The rest of the event system in crank works in node and reusing events between the browser and node could be really helpful down the road for server side rendering.

I'm happy to work on a PR for it!

@brainkim
Copy link
Member

I hope to have a custom event shim at some point, either here or in a helper library, but in the meantime the CustomEvent from event-target-shim works!

@ryhinchey
Copy link
Contributor Author

I don't see CustomEvent available as an export on event-target-shim. This issue is that doing new CustomEvent in node isn't supported.

@brainkim
Copy link
Member

Ahh I assumed we were pulling them from EventTargetShim but they’re from JSDOM 🤔

@ryhinchey
Copy link
Contributor Author

yah - JSDOM does a lot for us in tests :)

new CustomEvent needs to work in node & TS. We could make our own interface based on this since there are a bunch of things we don't need in node.

We'd then need to have a helper module that does something like this:

// CustomEvent.js
const getCustomEventClass = () => {
  const isBrowser = () => typeof window === 'object';
  
  if (isBrowser()) {
    return CustomEvent; // the browser's class
  }

  return NodeCustomEvent // our custom class 

}

export default getEventClass();

then you'd do something like import CustomEvent from './CustomEvent'; everywhere you want to use CustomEvent in Crank and I'm pretty sure this would keep jsdom happy as well

@brainkim brainkim added enhancement New feature or request help wanted Extra attention is needed labels Apr 29, 2020
@ryhinchey
Copy link
Contributor Author

I'm working on a fork of event-target-shim that will convert the project to typescript, remove a few things we don't need and add their CustomEvent to its exports

@brainkim
Copy link
Member

brainkim commented Apr 30, 2020

@ryhinchey Yeah, one of the things I didn’t like about the event-target-shim typings is that it extended dispatchEvent where you could just pass an object to dispatchEvent. I don’t mind that kind of API, I would just rather have a separate API like this.dispatch or something to keep it strictly to spec with the EventTarget class. In short, any EventTarget interface we define should exactly match the EventTarget interface provided by the DOM, as typed in TypeScript’s lib.dom.d.ts

Also, you might want to check out Deno’s implementation of EventTargets and events if you’re thinking about this (https://github.com/denoland/deno/blob/master/cli/js/web/event_target.ts). I think it’s more rigorous, and it’s written in TypeScript already, which is nice.

Also, I’m curious to hear your thoughts on my proposal that we dispatch any onevent props found in component props. This could be a good way to make the two styles of APIs more uniform.

@brainkim brainkim changed the title CustomEvent that works in node and the browser EventTarget + Event + CustomEvent in all environments May 25, 2020
@brainkim
Copy link
Member

nodejs/node#33556

An interesting pull request from Node.js

@jasnell
Copy link

jasnell commented May 25, 2020

Note that in the PRs current state, the Event constructor will not be exposed to userland. However, userland code will be able to call dispatchEvent with any object that has a type property whose value is a string

@brainkim
Copy link
Member

brainkim commented May 25, 2020

@jasnell That’s interesting! I worry about node having to ship a non-standard dispatchEvent in perpetuity but I understand why’d you make that decision. I know first-hand that the Event specification is actually a lot of work!

@brainkim
Copy link
Member

brainkim commented Jul 2, 2020

I managed to implement an EventTarget with capturing and bubbling for 0.2.0, without having to define an Event/CustomEvent class. I think Crank’s philosophy RE Event/CustomEvent is going to be bring your own. Not sure how much work that entails but could be a fun little project. Alternatively using the Event classes from JSDom seems to be fine, though that library is a little heavyweight if you just want the Event class.

@brainkim brainkim changed the title EventTarget + Event + CustomEvent in all environments Event + CustomEvent in all environments Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants