Makes the `ontouch*` events on window optional. #749

MicahZoltu · 2019-08-31T13:40:35Z

Browsers without touch support (e.g., Firefox/Chrome on desktop) will not have ontouch* events defined on window, so the current type definition is not correct.

MicahZoltu · 2019-08-31T13:40:58Z

Fixes microsoft/TypeScript#33176

orta · 2019-09-11T15:12:41Z

This makes sense, want to update the snapshots and this can be merged?

MicahZoltu · 2019-09-12T01:05:47Z

How does one "update the snapshots"? Is that a fancy way of saying "rebase on master"?

MicahZoltu · 2019-09-12T01:07:01Z

Ah, you are probably referring to the instructions here? https://github.com/microsoft/TSJS-lib-generator#contribution-guidelines

It makes me sad that those aren't done automatically as part of CI/CD. 😢 It means I'll have to actually clone this project to make this PR rather than just doing it in the GitHub UI.

MicahZoltu · 2019-09-12T01:20:36Z

Hmm, digging in on this it seems I need to add an overridingTypes.json entry, presumably on the window interface. Is there a way to say, "keep the auto-generated signature, but make it optional" or do I have to provide a copy of the signature with | undefined on it?

MicahZoltu · 2019-09-12T01:43:18Z

I have rebased the PR and applied the necessary change to overridingTypes.json in what I think is the proper way. I couldn't figure out how to make something optional other than by adding | undefined to the type signature. It is also worth noting that the global variable ontouchcancel will not be defined (different from undefined because JavaScript), yet the TS definition currently has it as declare var ontouchcancel: ... | null | undefined, which means if you do if (ontouchstart === undefined) TS will compile but you'll get a runtime exception I believe. I don't know how to resolve this.

saschanaz · 2019-09-27T09:46:45Z

inputfiles/overridingTypes.json

@@ -129,6 +129,18 @@
                    "property": {
                        "onerror": {
                            "override-type": "OnErrorEventHandler"
+                        },
+                        "ontouchcancel": {
+                            "override-type": "((this: Window, ev: TouchEvent) => any) | null | undefined"


Try replacing this line with nullable: 1.

Of course, why didn't I think that nullable: 1 would change a nullable type into a nullable | undefined type...

MicahZoltu · 2019-09-27T09:58:58Z

Rebased again and updated to use "nullable": 1 instead of the manual override.

saschanaz · 2019-09-27T09:59:47Z

You also need to run npm run build && npm run baseline-accept 😁

MicahZoltu · 2019-09-27T10:00:49Z

Bleck. I cannot run npm baseline-accept because it doesn't work on Windows. 😢 If a maintainer wants to run that, feel free. Or just crib this PR and make a new one. I just want the code fixed, don't care how it happens.

MicahZoltu · 2019-09-27T10:01:56Z

Ah, easy enough to replicate the baseline-accept step by hand. Fixed.

saschanaz · 2019-09-27T10:02:02Z

Could you try npm i cpx2 and run it again? Because I accidentally broke it 🙄

MicahZoltu · 2019-09-27T10:03:00Z

npm install cpx2 does appear to resolve the error. I already have matching baseline, so I'm just going to assume the copy did something. 😉

saschanaz · 2019-09-27T10:21:31Z

This PR now unexpectedly is no-op 🤯

I think what we want to do here is marking it as ontouchstart?: (whatever). ~~We should probably create a new way to do it because AFAIK there is no way to do it now.~~

saschanaz · 2019-09-27T10:23:11Z

inputfiles/overridingTypes.json

@@ -129,6 +129,18 @@
                    "property": {
                        "onerror": {
                            "override-type": "OnErrorEventHandler"
+                        },
+                        "ontouchcancel": {
+                            "nullable": 1


I got an ugly working hack: Try replacing this line with "name": "ontouchcancel?" and so on. This hack is currently being used for nonce?.

Hmm, I could have sworn that it was not a no-op when I committed...

Ewe.... that is dirty... but it appears to work. Pushed.

saschanaz · 2019-09-27T10:32:58Z

baselines/dom.generated.d.ts

-declare var ontouchend: ((this: Window, ev: TouchEvent) => any) | null;
-declare var ontouchmove: ((this: Window, ev: TouchEvent) => any) | null;
-declare var ontouchstart: ((this: Window, ev: TouchEvent) => any) | null;
+declare var ontouchcancel?: ((this: Window, ev: TouchEvent) => any) | null;


Ah, no, this won't work.... Sorry to make you break code 🙄
It seems we need to modify emitter to support this properly.

Should I let this PR lapse until such work is complete, or return to the original solution that will break if the auto-generated types ever change?

Okay, I got a working way. Please fix src/emitter.ts line 620 as:

- const requiredModifier = p.required === undefined || p.required === 1 ? "" : "?"; + const requiredModifier = p.required === undefined || p.required === 1 || prefix ? "" : "?";

And use required: 0 on overridingTypes.json. I tried it and it works this time!

That change doesn't appear to update the global variables:

declare var ontouchcancel: ((this: Window, ev: TouchEvent) => any) | null; declare var ontouchend: ((this: Window, ev: TouchEvent) => any) | null; declare var ontouchmove: ((this: Window, ev: TouchEvent) => any) | null; declare var ontouchstart: ((this: Window, ev: TouchEvent) => any) | null;

Those need to be |undefined as well I believe?
in Firefox if you do >> ontouchcancel you get

ReferenceError: ontouchcancel is not defined

Updated PR with your suggested changes in case that is desired behavior.

Ah, you're right. We can modify it further:

} - const requiredModifier = p.required === undefined || p.required === 1 ? "" : "?"; + const required = p.required === undefined || p.required === 1; + const requiredModifier = required || prefix ? "" : "?"; pType = p.nullable ? makeNullable(pType) : pType; + if (!required && prefix) { + pType += " | undefined" + }

Updated. I look forward to the day when someone git-blames me for this and I have no idea what any of it does. 😛

Browsers without touch support (e.g., Firefox/Chrome on desktop) will not have `ontouch*` events defined on `window`, so the current type definition is not correct. package-lock was out of date and auto-updated on `npm install`. I can roll that back if desired.

sandersn · 2019-10-02T14:59:22Z

Do we still need this PR if feature detection code like (('ontouchstart' in window) || (window.navigator)) works in 3.6 and later?

MicahZoltu · 2019-10-02T15:53:06Z

I'm not familiar with the feature detection code you are referring to, but the current definition of Window in dom TypeScript lib is incorrect. So unless the feature detection thing you are referring to fixes the bug in the Window definition, this PR is still necessary.

saschanaz · 2019-10-02T16:01:12Z

I'm not sure the current types can break things given that the in type guard problem is now fixed.

if (window.ontouchstart === undefined) {
    window.ontouchstart // currently this becomes never
}

This is the only weird thing I could find now but it shouldn't break anything.

MicahZoltu · 2019-10-02T16:08:32Z

The current problem is that window.ontouchstart type definition isn't a union with undefined. This means that the following code will not give a compile time error, but it should:

declare const myWindow: Window
myWindow.ontouchstart(...) // should error at compile time with calling possibly undefined

saschanaz · 2019-10-02T16:09:39Z

This means that the following code will not give a compile time error

It will, because it's a union with null and null is not callable.

MicahZoltu · 2019-10-02T16:33:09Z

Ah, good point. However, in a desktop browser with no touch interface, window.ontouchcancel is undefined, not null. In TypeScript 3.7.0-beta, the following will compile (just tested) but I believe it will fail at runtime since myWindow.ontouchcancel is undefined, not null:

declare const touchEvent: TouchEvent
declare const myWindow: Window
if (myWindow.ontouchcancel !== null) {
	myWindow.ontouchcancel(touchEvent)
}

saschanaz · 2019-10-02T16:34:33Z

Hmmm technically yes... but why would anyone directly call a event listener?

MicahZoltu · 2019-10-02T17:18:42Z

🤷‍♂
Regardless of whether it is useful, I think that it should be correct.

sandersn · 2019-11-14T19:34:16Z

Seem reasonable to make it correct. If there are unforeseen problems that people run into, we have enough time to revert this change before 3.8.

MicahZoltu force-pushed the patch-1 branch from e57ffd8 to 947ab2c Compare September 12, 2019 01:41

saschanaz reviewed Sep 27, 2019

View reviewed changes

MicahZoltu force-pushed the patch-1 branch from 947ab2c to fd863a5 Compare September 27, 2019 09:57

MicahZoltu force-pushed the patch-1 branch from fd863a5 to a9a0768 Compare September 27, 2019 10:01

saschanaz reviewed Sep 27, 2019

View reviewed changes

MicahZoltu force-pushed the patch-1 branch from a9a0768 to 1025b40 Compare September 27, 2019 10:31

saschanaz reviewed Sep 27, 2019

View reviewed changes

MicahZoltu force-pushed the patch-1 branch from 1025b40 to 2daf422 Compare September 27, 2019 11:02

MicahZoltu force-pushed the patch-1 branch from 2daf422 to 5c279b3 Compare September 27, 2019 11:20

saschanaz approved these changes Sep 27, 2019

View reviewed changes

saschanaz mentioned this pull request Oct 2, 2019

window.ontouchstart should be optional microsoft/TypeScript#33176

Closed

sandersn merged commit e1811c5 into microsoft:master Nov 14, 2019

Makes the ontouch* events on window optional. #749

Makes the ontouch* events on window optional. #749

Uh oh!

Conversation

MicahZoltu commented Aug 31, 2019

Uh oh!

MicahZoltu commented Aug 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orta commented Sep 11, 2019

Uh oh!

MicahZoltu commented Sep 12, 2019

Uh oh!

MicahZoltu commented Sep 12, 2019

Uh oh!

MicahZoltu commented Sep 12, 2019

Uh oh!

MicahZoltu commented Sep 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MicahZoltu commented Sep 27, 2019

Uh oh!

saschanaz commented Sep 27, 2019

Uh oh!

MicahZoltu commented Sep 27, 2019

Uh oh!

MicahZoltu commented Sep 27, 2019

Uh oh!

saschanaz commented Sep 27, 2019

Uh oh!

MicahZoltu commented Sep 27, 2019

Uh oh!

saschanaz commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschanaz Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschanaz Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented Oct 2, 2019

Uh oh!

MicahZoltu commented Oct 2, 2019

Uh oh!

saschanaz commented Oct 2, 2019

Uh oh!

MicahZoltu commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschanaz commented Oct 2, 2019

Uh oh!

MicahZoltu commented Oct 2, 2019

Uh oh!

saschanaz commented Oct 2, 2019

Uh oh!

MicahZoltu commented Oct 2, 2019

Uh oh!

sandersn commented Nov 14, 2019

Makes the `ontouch*` events on window optional. #749

Makes the `ontouch*` events on window optional. #749

MicahZoltu commented Aug 31, 2019 •

edited

Loading

saschanaz commented Sep 27, 2019 •

edited

Loading

saschanaz Sep 27, 2019 •

edited

Loading

saschanaz Sep 27, 2019 •

edited

Loading

MicahZoltu commented Oct 2, 2019 •

edited

Loading