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

CALL-3878: Migrate to TS and add CJS & ESM Builds #174

Draft
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

hemang-thakkar
Copy link
Contributor

Description

Jira Ticket: CALL-3878

Merge Checklist

Q A
Adds Documentation?
Any Dependency Changes?
Patch: Bug Fix?
Minor: New Feature?
Major: Breaking Change?

BRAVE Checklist

  • I have read the BRAVE checklist and confirmed if the following is necessary.
Q A
Backwards Compatible?
Rollout/Rollback Plan?
Automated test coverage?
Verified that changes work?
Expect Dependencies to Fail?

esme and others added 30 commits June 16, 2023 09:46
* Create incoming call started method to test inbound calling
* Refactor constants

* Refactor version constant

* 0.1.6-alpha.0

* Remove sdkVersion prop from initialized event

* 0.1.6-alpha.1

* Introduce inbound calling availability events

* Test new events

* Configure prettier and format the demo-minimal-js html. Improve accessibility and SEO.

* Use semantic html and style buttons

* Refactor switch case into onclick methods and remove logic that waits for DOM content loaded since we are using defer to load the JS

* Rename id

* Specify encoding

* Match SDK event name

* sort props

* Initialize as logged out

* Add user availability events

* Prevent window overflow

* Allow user to change availability during a call.

* disable availability buttons during a call.

* 0.1.7-alpha.0

* Use new alpha version
Create event handlers for create_engagement_succeeded and create_engagement_failed
* Initialize SDK on ready event

* Save userAvailable state in the demo widget

* Allow user to start an incoming call asynchronously for testing (#146)
* Initialize SDK on ready event

* Save userAvailable state in the demo widget

* Allow user to start an incoming call asynchronously for testing (#146)

* Revert "Allow user to start an incoming call asynchronously for testing (#146)"

This reverts commit 9800470.

---------

Co-authored-by: Esme Ling <[email protected]>
Co-authored-by: Esme <[email protected]>
* Add events for caller id matching

* Add caller id match payload

* Add debugMessageType argument to logging

* Add incomingCall event handler to README

* Log last sync event
* Add support for inbound calling to react demo app

* Add unit tests for Incoming Screen

* Unit tests

* Fix useCti hook to store incomingNumber

* Feedback fixes # 1

* Feedback fixes #2

* Feedback fixes
* Send navigate_to_record event when we receive an existing call id

* Fix console debug message const

* Navigate to record page in the react demo app (#165)

* Update README with instructions for redirect

* 0.2.1-alpha.3

* Use next tag of SDK
…166)

* React Demo App: Set incoming number and contact info in localstorage on redirect

* feedback fixes
@hemang-thakkar hemang-thakkar self-assigned this Feb 9, 2024
@hemang-thakkar hemang-thakkar requested a review from a team as a code owner February 9, 2024 19:42
@hemang-thakkar hemang-thakkar marked this pull request as draft February 9, 2024 19:42
Copy link

github-actions bot commented Feb 9, 2024

PR Preview Action v1.4.4
🚀 Deployed preview to https://HubSpot.github.io/calling-extensions-sdk/pr-preview/pr-174/
on branch gh-pages at 2024-02-09 19:54 UTC

@alonso-cadenas
Copy link
Collaborator

These changes mean we are coupling our inbound calling release with the TS migration. Here are a few concerns:

  • Is there more risk for something going wrong?
  • We're not going to GA with inbound calling realistically until Q3 2024. Are we okay with the TS migration being blocked until then?

@esme
Copy link
Contributor

esme commented Feb 9, 2024

These changes mean we are coupling our inbound calling release with the TS migration.

Yeah these are valid concerns. We could do the following to mitigate risk:

  • Split this PR into smaller PRs with files that have varying levels of impact on the release, i.e. lower risk changes such as the React Demo app
  • Maintain backwards compatibility by duplicating files in JS and TS
  • Publish an alpha version to test it with other providers in inbound calling beta

@hemang-thakkar
Copy link
Contributor Author

These changes mean we are coupling our inbound calling release with the TS migration.

Yeah these are valid concerns. We could do the following to mitigate risk:

  • Split this PR into smaller PRs with files that have varying levels of impact on the release, i.e. lower risk changes such as the React Demo app
  • Maintain backwards compatibility by duplicating files in JS and TS
  • Publish an alpha version to test it with other providers in inbound calling beta

I agree we should split this up into multiple PRs and incrementally migrate. I doubt we would need to duplicate files as we are exporting from index.js. So, customers always import from the package which points to index file. Internal structure shouldnt cause problem. I was not aware of realistic date for inbound GA. But, from this PR, we can split up our story into tasks and do incremental migration release.

@hemang-thakkar
Copy link
Contributor Author

These changes mean we are coupling our inbound calling release with the TS migration. Here are a few concerns:

  • Is there more risk for something going wrong?
  • We're not going to GA with inbound calling realistically until Q3 2024. Are we okay with the TS migration being blocked until then?

Ideally breaking change warrants for major version release. But, our timeline might not be helpful here.

@hemang-thakkar hemang-thakkar changed the base branch from master to proj-inbound-calling February 12, 2024 15:47
Base automatically changed from proj-inbound-calling to master March 4, 2024 19:46
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.

3 participants