Skip to content

Commit

Permalink
Fix TypeScript types
Browse files Browse the repository at this point in the history
  • Loading branch information
@fregante
fregante committed Mar 7, 2019
1 parent ede7235 commit a34d85f
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 9 deletions.
5 changes: 3 additions & 2 deletions index.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@
* Delegates event to a selector.
*/
declare type CombinedElements = EventTarget | EventTarget[] | NodeListOf<Element> | String;
declare const _default: (elements: CombinedElements, selector: string, type: string, callback: () => any, useCapture: boolean | AddEventListenerOptions) => any;
export = _default;
declare function delegate(selector: string, type: string, callback?: Function, useCapture?: boolean | AddEventListenerOptions): object;
declare function delegate(elements: CombinedElements, selector: string, type: string, callback?: Function, useCapture?: boolean | AddEventListenerOptions): object;
export = delegate;

This comment has been minimized.

Sign in to view

Copy link
@sindresorhus

sindresorhus Mar 8, 2019

Collaborator

export = is the legacy TS syntax. You should be using this:

export type CombinedElements = EventTarget | EventTarget[] | NodeListOf<Element> | string;

/**
 * Delegates event to a selector.
 */
declare function delegate(selector: string, type: string, callback?: Function, useCapture?: boolean | AddEventListenerOptions): object;

/**
 * Delegates event to a selector.
 */
declare function delegate(elements: CombinedElements, selector: string, type: string, callback?: Function, useCapture?: boolean | AddEventListenerOptions): object;

export default delegate;

You might be able to export the functions directly. I'm not sure if TS supports overloaded exports.

Also note that it should be string, not String.

And I would recommend typing Function to be more specific.

And the return value should be more explicitly typed (The .destroy()) method.

This comment has been minimized.

Sign in to view

Copy link
@sindresorhus

sindresorhus Mar 8, 2019

Collaborator

I just realized the source is TS, but my comments still apply, just to the source itself.

This comment has been minimized.

Sign in to view

Copy link
@fregante

fregante Mar 8, 2019 •
edited
Loading

Author Owner

export = is the legacy TS syntax. You should be using this:

export default translates to exports['default']

You might be able to export the functions directly. I'm not sure if TS supports overloaded exports.

This is actually a generated file by TypeScript with declaration: true. I had module.exports = in index.ts but the overloading didn't work.

Also note that it should be string, not String.

Where is String?

And I would recommend typing Function to be more specific.
And the return value should be more explicitly typed (The .destroy()) method.

👌

This comment has been minimized.

Sign in to view

Copy link
@fregante

fregante Mar 8, 2019 •
edited
Loading

Author Owner

And the return value should be more explicitly typed (The .destroy()) method.

I haven't done that because I intend to add a delegate.off method to match removeEventListener instead. Now it's possible because I keep track of listeners and can match the user's callback function with the delegate handler.

This comment has been minimized.

Sign in to view

Copy link
@fregante

fregante Mar 8, 2019

Author Owner

I opened #1

This comment has been minimized.

Sign in to view

Copy link
@sindresorhus

sindresorhus Mar 8, 2019

Collaborator

export default translates to exports['default']

The trick is to also add module.exports: https://github.com/sindresorhus/is/blob/79144f95425e75a601e208d83cc0eb6c8ad7565d/source/index.ts#L369-L373

Where is String?

a34d85f#diff-ed009b6b86b017532ef0489c77de5100R66

This comment was marked as off-topic.

Sign in to view

Copy link
@fregante

fregante Mar 8, 2019

Author Owner

That translates to:

exports.default = delegate;
module.exports = delegate;

So while we're exporting default in the types, it does not exist in the final CJS file. Is that ok?

This comment was marked as off-topic.

Sign in to view

Copy link
@fregante

fregante Mar 8, 2019

Author Owner

I think that while export = is legacy, as long as it works, it's more correct than exporting a default that doesn't exist, as long as we're targeting CJS and not ES Modules

This comment has been minimized.

Sign in to view

Copy link
@sindresorhus

sindresorhus Mar 8, 2019

Collaborator

So while we're exporting default in the types, it does not exist in the final CJS file. Is that ok?

I'm aware, but I feel that's an ok compromise. When everything moves to ESM this will be moot anyway, so it's just a transitional quirk.

This comment has been minimized.

Sign in to view

Copy link
@sindresorhus

sindresorhus Mar 8, 2019

Collaborator

I think that while export = is legacy, as long as it works, it's more correct than exporting a default that doesn't exist, as long as we're targeting CJS and not ES Modules

I disagree. You're targeting both CommonJS and ESM. There's also a readability issue. People not familiar with TS will find it really weird.

This comment has been minimized.

Sign in to view

Copy link
@fregante

fregante Mar 8, 2019 •
edited
Loading

Author Owner

I'm not targeting ESM because if I do I have to also have to add a JS file with export default and the module field but that would break regular requires: webpack/webpack#4742

In short if there's true ESM support, then CJS requires will have to be const delegate = require('delegate-it').default

If I leave it as a regular CJS module, all current ESM tooling will map import delegate from 'delegate-it' to module.export, but the opposite isn't true as shown in that webpack issue.

However your suggestion was alright as:

  • it's ok for TypeScript
  • the final CJS file has a module.exports = fn
  • even if no tool will use the default export from a CJS file, it technically exists so the definition is correct.

This comment has been minimized.

Sign in to view

Copy link
@fregante

fregante Mar 8, 2019

Author Owner

The clearest option would be to do away with the default exports, like many are suggesting in the meta issue you opened recently.

import {delegate} from 'delegate-it';
const {delegate} = require('delegate-it');

Doesn't seem too bad. What do you think (generally speaking)?

This comment has been minimized.

Sign in to view

Copy link
@sindresorhus

sindresorhus Mar 8, 2019

Collaborator

Like I commented in the meta issue discussion; doesn't really make sense to me to use named import in this case.
27 changes: 21 additions & 6 deletions index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ function _delegate(
element: EventTarget,
selector: string,
type: string,
callback: () => any,
useCapture: boolean | AddEventListenerOptions
callback?: Function,
useCapture?: boolean | AddEventListenerOptions
) {
const listenerFn = event => {
event.delegateTarget = event.target.closest(selector);
Expand Down Expand Up @@ -64,12 +64,25 @@ function _delegate(
* Delegates event to a selector.
*/
type CombinedElements = EventTarget | EventTarget[] | NodeListOf<Element> | String;
export = function delegate(
function delegate(
selector: string,
type: string,
callback?: Function,
useCapture?: boolean | AddEventListenerOptions
): object;
function delegate(
elements: CombinedElements,
selector: string,
type: string,
callback: () => any,
useCapture: boolean | AddEventListenerOptions
callback?: Function,
useCapture?: boolean | AddEventListenerOptions
): object;
function delegate(
elements,
selector,
type,
callback?,
useCapture?
) {
// Handle the regular Element usage
if (typeof (elements as EventTarget).addEventListener === 'function') {
Expand All @@ -78,7 +91,7 @@ export = function delegate(

// Handle Element-less usage, it defaults to global delegation
if (typeof type === 'function') {
return _delegate(document, elements as string, selector as string, type as () => any, callback as boolean | AddEventListenerOptions);
return _delegate(document, elements as string, selector as string, type as Function, callback as boolean | AddEventListenerOptions);
}

// Handle Selector-based usage
Expand All @@ -91,3 +104,5 @@ export = function delegate(
return _delegate(element, selector, type, callback, useCapture);
});
}

export = delegate;
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"scripts": {
"test": "xo !index.js && ava && tsc",
"prepublish": "tsc",
"preversion": "tsc"
"build": "tsc"
},
"xo": {
"envs": [
Expand Down

0 comments on commit a34d85f

Please sign in to comment.