0kku / destiny

A reactive UI library for JavaScript and TypeScript
Open Software License 3.0
53 stars 6 forks source link

Add a type index so that types work properly when installed via npm #18

Closed BobobUnicorn closed 3 years ago

BobobUnicorn commented 3 years ago

I'm not a particular fan of the /// , and it's explicitly discouraged by the TypeScript docs anyway. See 1. However, I don't believe there's another option that makes this work, so 🤷🏻‍♀️.

0kku commented 3 years ago

It's pretty weird that the path is a "relative" path, yet it's not relative to the file's path where it's being referenced, but instead relative to package.json. Is this intentional?

0kku commented 3 years ago

@ebebbington's PR proposes it to be changed to an explicit import, which we may have to do to support Deno, but I suppose this would be an all right stop gap measure. I presume Deno doesn't like reference comments either.

BobobUnicorn commented 3 years ago

It's pretty weird that the path is a "relative" path, yet it's not relative to the file's path where it's being referenced, but instead relative to package.json. Is this intentional?

The file is at package root :p I can move it if you like.

BobobUnicorn commented 3 years ago

@ebebbington's PR proposes it to be changed to an explicit import, which we may have to do to support Deno, but I suppose this would be an all right stop gap measure. I presume Deno doesn't like reference comments either.

So, if I understand this correctly, ShadowRoot & CSSStyleSheet have to be explicitly imported to get the type augmentations?

0kku commented 3 years ago

It's pretty weird that the path is a "relative" path, yet it's not relative to the file's path where it's being referenced, but instead relative to package.json. Is this intentional?

The file is at package root :p I can move it if you like.

That'd be nicer. It didn't even occur to me that it wouldn't be in the src folder. Is there a reason you made a new file instead of just referencing the file in mod.ts?

BobobUnicorn commented 3 years ago

It's pretty weird that the path is a "relative" path, yet it's not relative to the file's path where it's being referenced, but instead relative to package.json. Is this intentional?

The file is at package root :p I can move it if you like.

That'd be nicer. It didn't even occur to me that it wouldn't be in the src folder. Is there a reason you made a new file instead of just referencing the file in mod.ts?

I could have, I suppose. I didn't really want to put a reference path in that file though; seems a little gross.

0kku commented 3 years ago

I could have, I suppose. I didn't really want to put a reference path in that file though; seems a little gross.

Seems gross either way, and it seems weird to have two separate roots for it, does it not? I guess I'm just worried that it'd cause some unforeseen problems with editors picking the wrong one or the wrong one getting picked for bundling.

0kku commented 3 years ago

@ebebbington's PR proposes it to be changed to an explicit import, which we may have to do to support Deno, but I suppose this would be an all right stop gap measure. I presume Deno doesn't like reference comments either.

So, if I understand this correctly, ShadowRoot & CSSStyleSheet have to be explicitly imported to get the type augmentations?

Seems so. Doesn't look like Deno likes references or global declarations (unless I've misunderstood) — though this isn't a concern yet for this PR. But ideally these would just be added to lib.d.ts sooner or later, and presumably that'll eventually happen and we won't need to have these annoying global declarations for stuff that should already be there anyway.

BobobUnicorn commented 3 years ago

I could have, I suppose. I didn't really want to put a reference path in that file though; seems a little gross.

Seems gross either way, and it seems weird to have two separate roots for it, does it not? I guess I'm just worried that it'd cause some unforeseen problems with editors picking the wrong one or the wrong one getting picked for bundling.

:woman_shrugging: Done

BobobUnicorn commented 3 years ago

fixed lint issue

0kku commented 3 years ago

All seems good to me now