adrianhopebailie / web-monetization

Web Monetization Explainer and Specification for submission to the WICG
Other
18 stars 3 forks source link

Merge MonetizationStartEvent and MonetizationProgressEvent? #22

Closed marcoscaceres closed 4 years ago

marcoscaceres commented 4 years ago

I'm wondering if we can merge MonetizationProgressEvent and MonetizationStartEvent into a single MonetizationEvent? MonetizationEvent could then be of type "start" and "progress".

adrianhopebailie commented 4 years ago

We could probably go one further and just have a single event type and combine the data carried on the two events.

sublimator commented 4 years ago

@marcoscaceres That very much does seem to be the prevailing convention: image

Are the eventInitDict interfaces different for the various KeyboardEvent/MouseEvent types ? Seems not.

We could probably go one further combine the data carried on the two events.

What do you mean by that exactly? Start is like a subset of progress in that it is the first packet? What about the new monetizationstop and monetizationpending events ? They have different fields.

marcoscaceres commented 4 years ago

What do you mean by that exactly? Start is like a subset of progress in that it is the first packet?

Sure, the question is if the different field can just be ignored or nulled out when not "start".

What about the new monetizationstop and monetizationpending events ? They have different fields.

Same as above... if we can consolidate them, or pull the information from somewhere else, then that's preferable. Basically, we want to avoid having multiple even definitions unless we absolutely have too (we may have too... but want to make sure we do the due diligence and are left with no other option).

sublimator commented 4 years ago

Can Web-IDL support discrimination of required eventInitDict fields based on type ?

Assuming it can't, if we kept with the prevailing convention, would need two types of Events ... (forget the names, they are just illustrative)

MonetizationStatusEvent = (Start|Stop|Pending) MonetizationPaymentEvent (Progress with assetCode/assetScale/amount)

The StatusEvents would have requestId/paymentPointer And the PaymentEvent would ALSO have the assetCode/assetScale/amount ...

Messing around with some TypeScript definitions right now.

marcoscaceres commented 4 years ago

Messing around with some TypeScript definitions right now.

Ok, will see what you come up with...

sublimator commented 4 years ago
export interface CommonEventInit extends EventInit {
  // Web-Monetization-Id header present in the SPSP request
  readonly requestId: string
  // The meta[@name="monetization"] @content value
  readonly paymentPointer: string
}

interface PaymentEventFields {
  // The amount * received * at the destination specified in the SPSP endpoint
  readonly amount: string
  readonly assetCode: string
  readonly assetScale: number
}

type Nullable<T> = {
  [P in keyof T]: T[P] | null
}

interface PaymentEventInit extends CommonEventInit, PaymentEventFields {}

class MonetizationEvent extends Event
  implements CommonEventInit, Nullable<PaymentEventFields> {
  readonly paymentPointer: string
  readonly requestId: string
  readonly type: 'start' | 'pending' | 'stop' | 'progress'
  readonly assetCode: string | null = null
  readonly assetScale: number | null = null
  readonly amount: string | null = null

  constructor(type: 'progress', eventInit: PaymentEventInit)
  constructor(type: 'start' | 'pending' | 'stop', eventInit: CommonEventInit)
  constructor(
    type: 'start' | 'pending' | 'stop' | 'progress',
    eventInit: CommonEventInit | PaymentEventInit
  ) {
    super(type, eventInit)
    this.paymentPointer = eventInit.paymentPointer
    this.requestId = eventInit.requestId
    this.type = type
    const asPayment = eventInit as PaymentEventInit
    if (asPayment.amount) {
      this.assetCode = asPayment.assetCode
      this.assetScale = asPayment.assetScale
      this.amount = asPayment.amount
    }
  }
}

const pending: MonetizationEvent = new MonetizationEvent('pending', {
  requestId: '1',
  paymentPointer: '$x'
})

const progress: MonetizationEvent = new MonetizationEvent('progress', {
  requestId: '1',
  paymentPointer: '$x',
  assetCode: 'USD',
  assetScale: 1,
  amount: '10'
})

You are referring to something like that ?

I am not sure how to make TypeScript 'know' that progress should have not nulled fields ... You can't extend from union types ...

image

sublimator commented 4 years ago

Basically would be nice to have type narrowing: image

So inside the if block the assetScale is non null when progress ...

There may be some way you can do it ...

sublimator commented 4 years ago

Maybe the pragmatic choice would be to just have the types non nullable, but in practice nullable:

  // Using assertion operator to support strict mode
  readonly assetCode!: string
  readonly assetScale!: number
  readonly amount!: string 

edit: Would probably prefer 2 discrete event types

marcoscaceres commented 4 years ago

I recall @adrianhopebailie was suggesting we might drop asset* and amount and use PaymentCurrencyAmount from the Payment Request API instead? apologies if I'm misremembering.

If that's the case, what ever property will hold that could potentially be nullable in the event for "start" and "stop" and always set for "progress" (and whatever it needs to be for "pending").

sublimator commented 4 years ago

Oh yeah, I forgot about that :) A similar deal though. You either type it as amount: T | null (with associated poor developer ergonomics) or just simply amount: T ( a lie, it could be null on some event types )

adrianhopebailie commented 4 years ago

Kamino closed and cloned this issue to interledger/webmonetization.org