Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update event-target-shim and typescript versions and typescript fixes #76

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

bgila
Copy link
Contributor

@bgila bgila commented Mar 30, 2022

resolves #62 which was causing an infinite recursion when an event is dispatched.

  • updated event-target-shim and typescript versions, made a couple typescript fixes in upchunk.ts to ensure tests still pass.

  • not very knowledgeable with typescript, so if there's better ways to resolve the typescript issues that appear when upgrading typescript versions, would appreciate help on those

  • if we can merge this in, it'll allow Sprig to use Mux's upchunk directly instead of our fork, which we've been using for some months now.

Copy link
Contributor

@mmcc mmcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, thank you! One question on a type, but otherwise I think we can ship this.

@@ -128,7 +128,7 @@ export class UpChunk {
private dispatch(eventName: EventName, detail?: any) {
const event = new CustomEvent(eventName, { detail });

this.eventTarget.dispatchEvent(event);
this.eventTarget.dispatchEvent(event as any);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be CustomEvent instead of any?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the quick review!

it's not working for me -- here are the error messages I get:

barakgila@Baraks-MacBook-Pro upchunk (bg-update-event-target-shim) $ yarn build
yarn run v1.22.17
$ yarn clean && yarn lint && yarn test && webpack --config webpack.prod.js && yarn pack
$ rm -rf ./dist
$ tslint --project .
no-unused-variable is deprecated. Since TypeScript 2.9. Please use the built-in compiler checks instead.
$ jest
ts-jest[versions] (WARN) Version 4.6.3 of typescript installed has not been tested with ts-jest. If you're experiencing issues, consider using a supported version (>=3.8.0 <4.0.0-0). Please do not report issues in ts-jest if you are using unsupported versions.
ts-jest[versions] (WARN) Version 4.6.3 of typescript installed has not been tested with ts-jest. If you're experiencing issues, consider using a supported version (>=3.8.0 <4.0.0-0). Please do not report issues in ts-jest if you are using unsupported versions.
 FAIL  src/upchunk.spec.js
  ● Test suite failed to run

    src/upchunk.ts:131:36 - error TS2769: No overload matches this call.
      Overload 1 of 2, '(event: never): boolean', gave the following error.
        Argument of type 'CustomEvent<any>' is not assignable to parameter of type 'never'.
      Overload 2 of 2, '(event: Event<string>): boolean', gave the following error.
        Argument of type 'CustomEvent<any>' is not assignable to parameter of type 'Event<string>'.
          Types of property 'target' are incompatible.
            Type 'EventTarget | null' is not assignable to type 'EventTarget<Record<string, Event<string>>, "standard"> | null'.
              Type 'EventTarget' is not assignable to type 'EventTarget<Record<string, Event<string>>, "standard">'.
                Types of property 'addEventListener' are incompatible.
                  Type '(type: string, callback: EventListenerOrEventListenerObject | null, options?: boolean | AddEventListenerOptions | undefined) => void' is not assignable to type '{ <T extends string>(type: T, callback?: EventListener<EventTarget<Record<string, Event<string>>, "standard">, Event<string>> | null | undefined, options?: AddOptions | undefined): void; (type: string, callback?: EventListener<...> | ... 1 more ... | undefined, options?: AddOptions | undefined): void; <T extends str...'.
                    Types of parameters 'options' and 'options' are incompatible.
                      Type 'AddOptions | undefined' is not assignable to type 'boolean | AddEventListenerOptions | undefined'.
                        Type 'AddOptions' is not assignable to type 'boolean | AddEventListenerOptions | undefined'.
                          Type 'AddOptions' is not assignable to type 'AddEventListenerOptions'.
                            Types of property 'signal' are incompatible.
                              Type 'AbortSignal | null | undefined' is not assignable to type 'AbortSignal | undefined'.
                                Type 'null' is not assignable to type 'AbortSignal | undefined'.

    131     this.eventTarget.dispatchEvent(event as CustomEvent);
                                           ~~~~~~~~~~~~~~~~~~~~

 FAIL  src/upchunk.spec.ts
  ● Test suite failed to run

    src/upchunk.ts:131:36 - error TS2769: No overload matches this call.
      Overload 1 of 2, '(event: never): boolean', gave the following error.
        Argument of type 'CustomEvent<any>' is not assignable to parameter of type 'never'.
      Overload 2 of 2, '(event: Event<string>): boolean', gave the following error.
        Argument of type 'CustomEvent<any>' is not assignable to parameter of type 'Event<string>'.
          Types of property 'target' are incompatible.
            Type 'EventTarget | null' is not assignable to type 'EventTarget<Record<string, Event<string>>, "standard"> | null'.
              Type 'EventTarget' is not assignable to type 'EventTarget<Record<string, Event<string>>, "standard">'.
                Types of property 'addEventListener' are incompatible.
                  Type '(type: string, callback: EventListenerOrEventListenerObject | null, options?: boolean | AddEventListenerOptions | undefined) => void' is not assignable to type '{ <T extends string>(type: T, callback?: EventListener<EventTarget<Record<string, Event<string>>, "standard">, Event<string>> | null | undefined, options?: AddOptions | undefined): void; (type: string, callback?: EventListener<...> | ... 1 more ... | undefined, options?: AddOptions | undefined): void; <T extends str...'.
                    Types of parameters 'options' and 'options' are incompatible.
                      Type 'AddOptions | undefined' is not assignable to type 'boolean | AddEventListenerOptions | undefined'.
                        Type 'AddOptions' is not assignable to type 'boolean | AddEventListenerOptions | undefined'.
                          Type 'AddOptions' is not assignable to type 'AddEventListenerOptions'.
                            Types of property 'signal' are incompatible.
                              Type 'AbortSignal | null | undefined' is not assignable to type 'AbortSignal | undefined'.
                                Type 'null' is not assignable to type 'AbortSignal | undefined'.

    131     this.eventTarget.dispatchEvent(event as CustomEvent);
                                           ~~~~~~~~~~~~~~~~~~~~

Test Suites: 2 failed, 2 total
Tests:       0 total
Snapshots:   0 total
Time:        1.61 s, estimated 2 s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
barakgila@Baraks-MacBook-Pro upchunk (bg-update-event-target-shim) $ yarn build
yarn run v1.22.17
$ yarn clean && yarn lint && yarn test && webpack --config webpack.prod.js && yarn pack
$ rm -rf ./dist
$ tslint --project .
no-unused-variable is deprecated. Since TypeScript 2.9. Please use the built-in compiler checks instead.
$ jest
ts-jest[versions] (WARN) Version 4.6.3 of typescript installed has not been tested with ts-jest. If you're experiencing issues, consider using a supported version (>=3.8.0 <4.0.0-0). Please do not report issues in ts-jest if you are using unsupported versions.
ts-jest[versions] (WARN) Version 4.6.3 of typescript installed has not been tested with ts-jest. If you're experiencing issues, consider using a supported version (>=3.8.0 <4.0.0-0). Please do not report issues in ts-jest if you are using unsupported versions.
 FAIL  src/upchunk.spec.js
  ● Test suite failed to run

    src/upchunk.ts:131:36 - error TS2769: No overload matches this call.
      Overload 1 of 2, '(event: never): boolean', gave the following error.
        Argument of type 'Event' is not assignable to parameter of type 'never'.
      Overload 2 of 2, '(event: Event<string>): boolean', gave the following error.
        Argument of type 'Event' is not assignable to parameter of type 'Event<string>'.
          Types of property 'target' are incompatible.
            Type 'EventTarget | null' is not assignable to type 'EventTarget<Record<string, Event<string>>, "standard"> | null'.
              Type 'EventTarget' is not assignable to type 'EventTarget<Record<string, Event<string>>, "standard">'.
                Types of property 'addEventListener' are incompatible.
                  Type '(type: string, callback: EventListenerOrEventListenerObject | null, options?: boolean | AddEventListenerOptions | undefined) => void' is not assignable to type '{ <T extends string>(type: T, callback?: EventListener<EventTarget<Record<string, Event<string>>, "standard">, Event<string>> | null | undefined, options?: AddOptions | undefined): void; (type: string, callback?: EventListener<...> | ... 1 more ... | undefined, options?: AddOptions | undefined): void; <T extends str...'.
                    Types of parameters 'options' and 'options' are incompatible.
                      Type 'AddOptions | undefined' is not assignable to type 'boolean | AddEventListenerOptions | undefined'.
                        Type 'AddOptions' is not assignable to type 'boolean | AddEventListenerOptions | undefined'.
                          Type 'AddOptions' is not assignable to type 'AddEventListenerOptions'.
                            Types of property 'signal' are incompatible.
                              Type 'AbortSignal | null | undefined' is not assignable to type 'AbortSignal | undefined'.
                                Type 'null' is not assignable to type 'AbortSignal | undefined'.

    131     this.eventTarget.dispatchEvent(event as Event);
                                           ~~~~~~~~~~~~~~

 FAIL  src/upchunk.spec.ts
  ● Test suite failed to run

    src/upchunk.ts:131:36 - error TS2769: No overload matches this call.
      Overload 1 of 2, '(event: never): boolean', gave the following error.
        Argument of type 'Event' is not assignable to parameter of type 'never'.
      Overload 2 of 2, '(event: Event<string>): boolean', gave the following error.
        Argument of type 'Event' is not assignable to parameter of type 'Event<string>'.
          Types of property 'target' are incompatible.
            Type 'EventTarget | null' is not assignable to type 'EventTarget<Record<string, Event<string>>, "standard"> | null'.
              Type 'EventTarget' is not assignable to type 'EventTarget<Record<string, Event<string>>, "standard">'.
                Types of property 'addEventListener' are incompatible.
                  Type '(type: string, callback: EventListenerOrEventListenerObject | null, options?: boolean | AddEventListenerOptions | undefined) => void' is not assignable to type '{ <T extends string>(type: T, callback?: EventListener<EventTarget<Record<string, Event<string>>, "standard">, Event<string>> | null | undefined, options?: AddOptions | undefined): void; (type: string, callback?: EventListener<...> | ... 1 more ... | undefined, options?: AddOptions | undefined): void; <T extends str...'.
                    Types of parameters 'options' and 'options' are incompatible.
                      Type 'AddOptions | undefined' is not assignable to type 'boolean | AddEventListenerOptions | undefined'.
                        Type 'AddOptions' is not assignable to type 'boolean | AddEventListenerOptions | undefined'.
                          Type 'AddOptions' is not assignable to type 'AddEventListenerOptions'.
                            Types of property 'signal' are incompatible.
                              Type 'AbortSignal | null | undefined' is not assignable to type 'AbortSignal | undefined'.
                                Type 'null' is not assignable to type 'AbortSignal | undefined'.

    131     this.eventTarget.dispatchEvent(event as Event);
                                           ~~~~~~~~~~~~~~

Test Suites: 2 failed, 2 total
Tests:       0 total
Snapshots:   0 total
Time:        1.583 s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm ok well I don't think that should block this from getting merged, so I'll merge and revisit after. Thanks again for the contribution!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, thank you! I'll stay tuned for your merge/version release processes

@bgila bgila requested a review from mmcc March 30, 2022 21:09
@mmcc mmcc merged commit 3716af3 into muxinc:master Mar 31, 2022
@mmcc
Copy link
Contributor

mmcc commented Mar 31, 2022

@bgila oh hey also, as a thanks for the PR I can shoot you a shirt or something along those lines, if you're interested just ping me via email (matt at mux)

@bgila bgila deleted the bg-update-event-target-shim branch March 31, 2022 16:06
@bgila
Copy link
Contributor Author

bgila commented Mar 31, 2022

@mmcc thank you for the quick review! Just wondering, what release cadence are y'all on -- when should I expect a new package version with the change?

I sent an email with my address too :D

@mmcc
Copy link
Contributor

mmcc commented Mar 31, 2022

I went ahead and cut a patch release for this, should be live on NPM already. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite recursion from old event-target-shim dependency
2 participants