intercom-web: Make intercomSettings optional to allow delete window.intercomSettings (New change in TS 4.0)

[tklepzig]

Please fill in this template.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change. (Run with npm test.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes:

  With TS 4.0, when in strictNullChecks mode (i.e., always, right??!), the operand of the delete operator MUST be any, unknown, never or be optional (i.e., containing undefined ); otherwise, the code doesn’t compile. Check out the PR for details.

(https://medium.com/javascript-in-plain-english/whats-new-with-typescript-4-0-beta-a2e674846ef3)

[typescript-bot]

@tklepzig Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

This PR doesn't modify any tests, so it's hard to know what's being fixed, and your changes might regress in the future. Have you considered adding tests to cover the change you're making? Including tests allows this PR to be merged by yourself and the owners of this module. This can potentially save days of time for you.

1 package in this PR

• intercom-webon npm, on unpkg

Code Reviews

This PR can be merged once it's reviewed.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Most recent commit is approved by type definition owners or DT maintainers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

---

Diagnostic Information: What the bot saw about this PR

{
"type": "info",
"now": "-",
"pr_number": 47191,
"author": "tklepzig",
"owners": [
"fongandrew",
"salbahra",
"onatm",
"bingo4508"
],
"dangerLevel": "ScopedAndUntested",
"headCommitAbbrOid": "81111f1",
"headCommitOid": "81111f13729ead758d07e0a0ac1a96eadb21b09a",
"mergeIsRequested": true,
"stalenessInDays": 0,
"lastPushDate": "2020-09-01T12:28:31.000Z",
"lastCommentDate": "2020-09-02T09:10:38.000Z",
"maintainerBlessed": true,
"reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/47191/files",
"hasMergeConflict": false,
"authorIsOwner": false,
"isFirstContribution": true,
"popularityLevel": "Well-liked by everyone",
"newPackages": [],
"packages": [
"intercom-web"
],
"files": [
{
"path": "types/intercom-web/index.d.ts",
"kind": "definition",
"package": "intercom-web"
}
],
"hasDismissedReview": false,
"ciResult": "pass",
"lastReviewDate": "2020-09-02T09:09:11.000Z",
"reviewersWithStaleReviews": [],
"approvalFlags": 2,
"isChangesRequested": false
}

[typescript-bot]

🔔 @fongandrew@salbahra@onatm @bingo4508 — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

[typescript-bot]

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊

	master	#47191	diff
Batch compilation
Memory usage (MiB)	36.6	36.6	+0.1%
Type count	2196	2196	0%
Assignability cache size	102	102	0%
Language service
Samples taken	52	52	0%
Identifiers in tests	52	52	0%
getCompletionsAtPosition
Mean duration (ms)	90.2	92.3	+2.3%
Mean CV	22.6%	23.7%
Worst duration (ms)	122.9	130.3	+6.1%
Worst identifier	horizontal_padding	horizontal_padding
getQuickInfoAtPosition
Mean duration (ms)	92.9	92.5	-0.4%
Mean CV	23.6%	24.5%	+3.5%
Worst duration (ms)	115.4	118.5	+2.7%
Worst identifier	intercomSettings	Intercom

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

[orta]

Note for reviewers: This would potentially be a breaking change for people using strict mode (because they now have to validate the existence of that value)

[onatm]

Hey @orta, thank you for letting us know that this would be a potential breaking change. I am not sure what would be the real impact for the future since the declaration is complete(-ish?).

[onatm]

@tklepzig Could you tell me what do you mean by delete window.intercomSettings? My knowledge of TS and intercom (or any frontend technology) could be a bit outdated atm.

[tklepzig]

@onatm Of course. We use Intercom for our customer's feedback. For that, we have to set it up during startup (https://developers.intercom.com/installing-intercom/docs/configuration). While doing this, we add the prop intercomSettings to the window object. In order to clean up things afterwards properly, we delete the property with delete window.intercomSettings.

Now with TS 4.0 a change has been introduced which allows delete only for optional properties in strict mode (https://medium.com/javascript-in-plain-english/whats-new-with-typescript-4-0-beta-a2e674846ef3#a545 and microsoft/TypeScript#37921). To fulfil this rule, I changed the property to an optional one.

See also a similar issue in another react library for intercom: nhagen/react-intercom#39

Does this answer your question?

[onatm]

that was a really nice explanation, thank you @tklepzig 👍 I am happy to move forward with this PR

[onatm]

Ready to merge

[typescript-bot]

I just published @types/intercom-web@2.8.9 to npm.

[tklepzig]

that was a really nice explanation, thank you @tklepzig 👍 I am happy to move forward with this PR

Thanks a lot!