Polymer / pwa-helpers

Small helper methods or mixins to help you build web apps.
BSD 3-Clause "New" or "Revised" License
439 stars 48 forks source link

No underscore in connect-mixin API #43

Closed keanulee closed 5 years ago

keanulee commented 5 years ago

0.9.0-pre.1 published to @next tag on NPM

eterna2 commented 5 years ago

@keanulee Hi, I can ask for the connect-mixin, can u add a AbstractConstructor type? Because we might want to pass in an abstract class?

PS: Can I ask why did u remove the mapStateToProps and mapDispatchToProps. I think it is useful.

keanulee commented 5 years ago

@eterna2 I couldn't figure out how to type an abstract constructor in TS (saw this but didn't work). Because of this, we're making LitElement itself not abstract anymore. I also couldn't return an abstract class in the mixin function.

We played with the idea of mapState/DispatchToProps, but ultimately abandoned it because it didn't play well with property typings (return { prop: 'foo' } wouldn't check that prop was on the element) and thought it was straightforward enough to just assign to this.prop in stateChanged. The implementation is simple though (Object.assign(this, maStateToProps(state))), so you could adopt this style if it better meets your needs.

eterna2 commented 5 years ago

@keanulee

To also accept abstract class, this is what I did.

ConstructorType|HTMLElement

But yeah, I can't return an abstract class either. But at least it let me pass in an abstract class. Might need to add an implements myself to mimic the abstract part.

keanulee commented 5 years ago

I'm confused - what exactly is the signature? It's currently:

type Constructor<T> = new(...args: any[]) => T;

export const connect =
  <S>(store: Store<S>) =>
  <T extends Constructor<HTMLElement>>(baseElement: T) =>

Since we can't return an abstract class anyway, the mixin would need to implement any abstract methods, and we wouldn't be able to do this for any arbitrary subclass of HTMLElement since we wouldn't know what's required. Not going to block this PR on this feature since we already don't allow abstract classes.

keanulee commented 5 years ago

@frankiefu PTAL and try out 0.9.0-pre.4. https://github.com/Polymer/pwa-starter-kit/pull/240 seems to be good.