GoogleChrome / dialog-polyfill

Polyfill for the HTML dialog element
BSD 3-Clause "New" or "Revised" License
2.45k stars 245 forks source link

Types file has a bug #204

Closed CamJN closed 3 years ago

CamJN commented 4 years ago

Sorry I can't really help fix it, as I'm not yet very good at typescript. But updating to 0.5.2 from 0.5.1 introduced this error at compile time:

[ ERROR ]  TypeScript: ./node_modules/dialog-polyfill/index.d.ts:17:16
           Cannot find name 'DialogPolyfill'. Did you mean 'dialogPolyfill'?

     L16:   */
     L17:  export default DialogPolyfill;
CamJN commented 4 years ago

If I had to guess I think you want to export DialogPolyfillType not DialogPolyfill from index.d.ts.

samthor commented 4 years ago

Thanks, I've made the update and released 0.5.3 đź‘Ť

I could probably set up an integration test for the d.ts file, but I'm not really an expert on that. Would accept PRs.

CamJN commented 4 years ago

While that fixed one error, it uncovered another.

When typescript compiles the following stencil.js component, the warning below is thrown, then when componentDidRender is called, it throws an error that dialogPolyfill is not defined, reverting to 0.5.1 resolves these issues both at compile and runtime.

import { Component, Element, Event, EventEmitter, Host, Listen, Method, h } from '@stencil/core';
import { ClickOutside } from "stencil-click-outside";
import dialogPolyfill from 'dialog-polyfill';

@Component({
  tag: 'dialog-polyfilled',
  styleUrl: 'dialog-polyfilled.css',
  shadow: true,
})
export class DialogPolyfilled {

  @Element() host: HTMLElement;
  @Event({  eventName: 'close' }) closer: EventEmitter;
  private dialog: HTMLDialogElement;

  @Listen('click', { capture: true })
  handleClick(e) {
    console.debug('click inside',e.target.nodeName);
    if (e?.target.nodeName === "BUTTON") {
      this.dialog?.close(e?.target.innerText);
      e.preventDefault();
      e.stopPropagation();
    }
  }

  @ClickOutside()
  handleClickOutside() {
    console.debug('click outside');
    this.close("reject");
  }

  @Method()
  async close(retVal) {
    return this.dialog?.close(retVal);
  }

  handleClose(e) {
    const value = e?.target.returnValue || e?.currentTarget.returnValue || "reject";
    console.debug(e?.type,value);
    this.closer.emit(value);
  }

  componentDidRender() {
    const d = this.dialog;
    dialogPolyfill.registerDialog(d);
    if (!d.open) { d.showModal() }
  }

  attributes() {
    return Object.fromEntries(Array.from(this.host.getAttributeNames()).map( name => [name,this.host.getAttribute(name)] ));
  }

  render() {
    const { class:classNames, ...attrs } = this.attributes();
    return (
      <Host>
        <dialog class={`${classNames} fixed`} {...attrs} ref={el => this.dialog = el as HTMLDialogElement} onClose={this.handleClose.bind(this)}>
          <slot></slot>
        </dialog>
      </Host>
    );
  }

}

Compile time warning:

[ WARN  ]  TypeScript: ./src/components/dialog-polyfilled/dialog-polyfilled.tsx:3:1
           'dialogPolyfill' is declared but its value is never read.

      L2:  import { ClickOutside } from "stencil-click-outside";
      L3:  import dialogPolyfill from 'dialog-polyfill';
samthor commented 4 years ago

So this is no longer an issue with the polyfill. You actually never call dialogPolyfill.registerDialog(dialogNode), which is required to make a dialog work anywhere there's no native support (i.e., anywhere but Chrome).

The polyfill doesn't have side effects just from being imported—you need to call it on on the <dialog>.

CamJN commented 4 years ago

I do call registerDialog in componentDidRender, however the import is messed up somehow, where it doesn't recognize the dialogPolyfill constant in the method being the same as the import.

componentDidRender() {
    const d = this.dialog;
    dialogPolyfill.registerDialog(d);
    if (!d.open) { d.showModal() }
  }

And like I said, this code works with 0.5.1

samthor commented 4 years ago

ugh, sorry - I completely missed that. I thought because previously we had no types the import was effectively undefined.

I'll have to run a TS sample to try to work out what's going on.

david2tm commented 4 years ago

Came here looking for this problem. I'm working with Angular. Doing import dialogPolyfill from 'dialog-polyfill' and calling lated dialogPolyfill.registerDialog(this.dialogRef). But it looks like webpack completely ignored that import... it just missing in transpiled js.

edit: it works with 0.5.1

yukulele commented 3 years ago

with:

import dp from 'dialog-polyfill';
dp.registerDialog(dialog);

I now have a ts(2693) dp only refers to a type, but is being used as a value here. error

@CamJN @samthor I think index.d.ts should not export DialogPolyfillType but DialogPolyfill

CamJN commented 3 years ago

Sorry for double posting, used the wrong account.

@yukulele it should not, that causes the original issue in this thread. index.d.ts defines the types and dist/dialog-polyfill.js exports the actual module, you should not be getting types when importing from the module into your app code, and likewise you should not get code when importing types from index.d.ts.

Moreover, look at https://github.com/GoogleChrome/dialog-polyfill/blob/master/index.d.ts there is no DialogPolyfill (note leading capital D) defined in that file to export.

yukulele commented 3 years ago

@CamJN I'm not sure to understand, is this code wrong?

import dp from 'dialog-polyfill';
dp.registerDialog(dialog);

It works as js but I get typescript error.

CamJN commented 3 years ago

@yukulele that code is fine, the issue is with how the typescript types are provided by the module, which is what causes the issue in typescript (and not javascript). I was just trying to explain to you why your suggested fix wouldn't work.

qurben commented 3 years ago

In the latest version the value exported from dialog-polyfill is a TypeScript interface, this should be a const.

This is a possible fix for the esm part of index.d.ts, I am not sure if it might break something else.

/**
 * If used as ESM, then we export a const of type "DialogPolyfillType" as the default type.
 */
declare const dialogPolyfill: DialogPolyfillType
export default dialogPolyfill;