Skip to content

Commit

Permalink
simplify things: remove lowclass/Mixin and use #private fields such t…
Browse files Browse the repository at this point in the history
…hat we can output declaration files

lowclass/Mixin and TypeScript "private" or "protected" access modifiers cause declaration output not to be possible, thus forcing us to point package.json types at src/index.ts instead of dist/index.d.ts, which can cause type errors in downstream consumers if they use different tsconfig.json settings when they type-check their applications (coupled with the fact that TypeScript will type-check library code if the library code types field does not point to declaration files, *even if* the user set `skipLibCheck` to `true`). Outputting declaration files and pointing `types` to them is the way to go for publishing libraries, but certain features of TypeScript do not work when declaration output is enabled, which is why we had to refactor. microsoft/TypeScript#35822

Now that lume/cli is updated, npm test fails. Needs a fix.
  • Loading branch information
trusktr committed Apr 19, 2021
1 parent 92e2e32 commit b02c052
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 32 deletions.
1 change: 0 additions & 1 deletion .prettierrc.js → .prettierrc.cjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
module.exports = {
tabWidth: 4,
useTabs: true,
semi: false,
singleQuote: true,
Expand Down
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"homepage": "http://github.com/lume/eventful#readme",
"type": "module",
"main": "dist/index.js",
"types": "src/index.ts",
"types": "dist/index.d.ts",
"exports COMMENT:": "This removes 'dist' from import statements, as well as replaces the 'main' field. See https://github.com/nodejs/node/issues/14970#issuecomment-571887546",
"exports": {
".": "./dist/index.js",
Expand Down Expand Up @@ -35,8 +35,8 @@
"lowclass": "^4.9.0"
},
"devDependencies": {
"@lume/cli": "^0.3.0",
"prettier": "^1.19.1"
"@lume/cli": "^0.5.0",
"prettier": "^2.0.0"
},
"repository": {
"type": "git",
Expand Down
38 changes: 34 additions & 4 deletions src/Eventful.test.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import Eventful from './Eventful.js'
import {Eventful} from './Eventful.js'

let eventCount = 0
let eventCount2 = 0

describe('Eventful', () => {
describe('provides an event pattern', () => {
it('triggers an event handler based on event names', () => {
const o = new Eventful()
const MyClass = Eventful()
const o = new MyClass()

const eventHandler = () => {
eventCount += 1
Expand Down Expand Up @@ -92,7 +93,8 @@ describe('Eventful', () => {
})

it('passes event payloads to event handlers', () => {
const o = new Eventful()
class MyClass extends Eventful() {}
const o = new MyClass()

let thePayload: string | number | undefined
let thePayload2: number | undefined
Expand Down Expand Up @@ -128,7 +130,8 @@ describe('Eventful', () => {
})

it('allows callbacks to be paired with contexts with which to be called', () => {
const o = new Eventful()
class MyClass extends Eventful() {}
const o = new MyClass()

let obj = {n: 0}
const o1 = {}
Expand Down Expand Up @@ -175,5 +178,32 @@ describe('Eventful', () => {

expect(obj.n).toBe(4)
})

it('allows us to check objects are instances of the mixin class or composed classes', () => {
const MyClass = Eventful()
class OtherClass extends Eventful() {}
class AnotherClass extends Eventful(Array) {}

let o = new MyClass
expect(o).toBeInstanceOf(Eventful)
expect(o).toBeInstanceOf(MyClass)
expect(o).not.toBeInstanceOf(OtherClass)
expect(o).not.toBeInstanceOf(AnotherClass)
expect(o).not.toBeInstanceOf(Array)

o = new OtherClass
expect(o).toBeInstanceOf(Eventful)
expect(o).not.toBeInstanceOf(MyClass)
expect(o).toBeInstanceOf(OtherClass)
expect(o).not.toBeInstanceOf(AnotherClass)
expect(o).not.toBeInstanceOf(Array)

o = new AnotherClass
expect(o).toBeInstanceOf(Eventful)
expect(o).not.toBeInstanceOf(MyClass)
expect(o).not.toBeInstanceOf(OtherClass)
expect(o).toBeInstanceOf(AnotherClass)
expect(o).toBeInstanceOf(Array)
})
})
})
53 changes: 33 additions & 20 deletions src/Eventful.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
import {Mixin, MixinResult, Constructor} from 'lowclass'
import type {Constructor} from 'lowclass'

// TODO @trusktr, make strongly typed event args. Combine with stuff in Events.ts (or similar).
// TODO, make strongly typed event args. Combine with stuff in Events.ts (or similar).

// TODO @trusktr, Make sure emit will not attempt to call event handlers removed
// TODO, Make sure emit will not attempt to call event handlers removed
// during emit (in place modification of listeners array during emit iteration
// will try to access undefined after the end of the array). Possibly use
// for..of with a Set instead, otherwise modify the iteration index manually.

// TODO @trusktr, an option to defer events, and batch them (so that 3 of the
// TODO, an option to defer events, and batch them (so that 3 of the
// same event and payload triggers only one event instead of three)

/**
* @mixin
* @class Eventful - An instance of Eventful emits events that code can
* subscribe to with callbacks. Events may optionally pass a payload to the
* callbacks.
Expand Down Expand Up @@ -41,12 +42,10 @@ import {Mixin, MixinResult, Constructor} from 'lowclass'
* rectangle.off("resize", onResize)
* ```
*/
export const Eventful = Mixin(EventfulMixin)
export interface Eventful extends InstanceType<typeof Eventful> {}
export default Eventful
export function Eventful<T extends Constructor>(Base: T = Object as any) {
class Eventful extends Base {
[isEventful] = isEventful

export function EventfulMixin<T extends Constructor>(Base: T) {
class Eventful extends Constructor(Base) {
/**
* @method on - Register a `callback` to be executed any
* time an event with name `eventName` is triggered by an instance of
Expand All @@ -60,13 +59,13 @@ export function EventfulMixin<T extends Constructor>(Base: T) {
* @param {any} context - An optional context to call the callback with. Passing no context is the same as subscribing `callback` for a `context` of `undefined`.
*/
on(eventName: string, callback: Function, context?: any) {
let eventMap = this.__eventMap
let eventMap = this.#eventMap

// @prod-prune
if (typeof callback !== 'function')
throw new Error('Expected a function in callback argument of Eventful#on.')

if (!eventMap) eventMap = this.__eventMap = new Map()
if (!eventMap) eventMap = this.#eventMap = new Map()

let callbacks = eventMap.get(eventName)

Expand All @@ -85,7 +84,7 @@ export function EventfulMixin<T extends Constructor>(Base: T) {
* @param {any} context - A context associated with `callback`. Not passing a `context` arg is equivalent to unsubscribing the `callback` for `context` of `undefined`.
*/
off(eventName: string, callback?: Function, context?: any) {
const eventMap = this.__eventMap
const eventMap = this.#eventMap

if (!eventMap) return

Expand All @@ -101,7 +100,7 @@ export function EventfulMixin<T extends Constructor>(Base: T) {

if (callbacks.length === 0) eventMap.delete(eventName)

if (eventMap.size === 0) this.__eventMap = null
if (eventMap.size === 0) this.#eventMap = null
}

/**
Expand All @@ -120,7 +119,7 @@ export function EventfulMixin<T extends Constructor>(Base: T) {
* @param {data} any - The data that is passed to each callback subscribed to the event.
*/
emit(eventName: string, data?: any) {
const eventMap = this.__eventMap
const eventMap = this.#eventMap

if (!eventMap) return

Expand All @@ -140,12 +139,24 @@ export function EventfulMixin<T extends Constructor>(Base: T) {
}
}

private __eventMap: Map<string, Array<[Function, any]>> | null = null
#eventMap: Map<string, Array<[Function, any]>> | null = null
}

return Eventful as MixinResult<typeof Eventful, T>
Eventful.prototype[isEventful] = isEventful

return Eventful
}

const isEventful = Symbol('isEventful')

Object.defineProperty(Eventful, Symbol.hasInstance, {
value(obj: any): boolean {
if (!obj || typeof obj !== 'object') return false
if (!(isEventful in obj)) return false
return true
},
})

/**
* @decorator
* @function emits - This is a decorator that when used on a property in a
Expand All @@ -171,7 +182,7 @@ function _emits(prototype: any, propName: string, descriptor: PropertyDescriptor
if (!(prototype instanceof Eventful))
throw new TypeError('The @emits decorator in only for use on properties of classes that extend from Eventful.')

const vName = 'emits_' + propName
const vName = Symbol('@emits: ' + propName)

// property decorators are not passed a descriptor (unlike decorators on accessors or methods)
let calledAsPropertyDecorator = false
Expand Down Expand Up @@ -226,7 +237,7 @@ function _emits(prototype: any, propName: string, descriptor: PropertyDescriptor
}

let initialValueIsSet = false
function emitEvent(this: Eventful) {
function emitEvent(this: EventfulInstance) {
this.emit(eventName, propName)
}

Expand Down Expand Up @@ -257,15 +268,15 @@ function _emits(prototype: any, propName: string, descriptor: PropertyDescriptor
originalSet!.call(this, newValue)

// TODO should we defer the event, or not? Perhaps provide an option, and defer by default.
Promise.resolve().then(emitEvent.bind(this as Eventful))
Promise.resolve().then(emitEvent.bind(this as EventfulInstance))
// emitEvent.call(this as Eventful)
},
}
: {
set(newValue: any) {
if (!initialValueIsSet) initialValueIsSet = true
;(this as any)[vName] = newValue
Promise.resolve().then(emitEvent.bind(this as Eventful))
Promise.resolve().then(emitEvent.bind(this as EventfulInstance))
},
}),
}
Expand All @@ -279,3 +290,5 @@ function _emits(prototype: any, propName: string, descriptor: PropertyDescriptor
// Weird, huh?
// This will all change with updates to the ES decorators proposal, https://github.com/tc39/proposal-decorators
}

type EventfulInstance = InstanceType<ReturnType<typeof Eventful>>
4 changes: 0 additions & 4 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
{
"extends": "./node_modules/@lume/cli/config/ts.config.json",
"compilerOptions": {
"declaration": false, // TODO, set back to true after removing class-factory mixins
"declarationMap": false
}
}

0 comments on commit b02c052

Please sign in to comment.