Throw error for missing client ID 👾

[spencersablan]

• 👽 Added more meaningful error handling - Before, without a client ID, if you tried to load the PayPal SDK, it would throw a generic error. It would be helpful to explicitly point out that the developer did not include the client ID.

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 9663659b6acb519f35635209ab06b6307d6ba210

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@paypal/paypal-js	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[spencersablan]

This is breaking the Playwright tests because the Playwright tests are expecting the request to /sdk/js even if there is no client ID. 😅

[spencersablan]

@jshawl @imbrian

I moved this logic down below processOptions() so that client ID would always be formatted as "client-id". Otherwise, it was throwing the error for: { "client-id": "abc" } or { clientID: "abc" }, which are both valid.

Although, @gregjopa brought up that we used to have logic to fetch the error message if /sdk/js returns a 400 status code: See here

This might be cool to reimplement since it would be able to log errors for missing client ID's, invalid client ID's, and so much more. It was removed for maintenance purposes but Greg said if we think it would be valuable, we can reimplement it.

I'd love to know your thoughts. Is this needed?

[gregjopa]

Although, @gregjopa brought up that we used to have logic to fetch the error message if /sdk/js returns a 400 status code: See here

If we want to add input validation to this library, I recommend we bring back the fallback fetch() approach that was originally in paypal-js. That will solve for things like missing and invalid client-ids, invalid query param keys and invalid query param values. It also keeps the JS SDK itself responsible for owning the validation logic. And this library is just a thin wrapper on top.

All the code for this is in version history along with tests. Happy to help bring it back. I think we learned the hard way with the JS SDK that any time you build an API that returns JavaScript, you should always return back with a 200 status code so the browser can read the response. Script tags will ignore the content of 400/500 http responses and will not execute the JS that gets returned. So this solution is a workaround to that problem.

At a high level, here's the logic:

1. If the script fails to get inserted into the DOM, use the script onerror callback to fetch the details of the error.
2. The fetch() call made to /sdk/js with the same exact query params will enable us to read the http response body.
3. When we get back a 400/500 error, parse the body as plain text and pull out the error message along with the debug id.
4. Return this error message and debug id when rejecting the problem.
5. Celebrate improving the dev ex 🎉

[github-actions[bot]]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

[github-actions[bot]]

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened.