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]

🦋 Changeset detected

Latest commit: 9663659

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

[imbrian]

Do we want to move this into loadCustomScript with the other validations? Seems like this might start breaking on incorrectly-implemented sites where another namespace exists and is used below.

[spencersablan]

I originally had it there but I realized that (per the README)loadCustomScript is just a generic script loader that should work with any URL. Whereas loadScript is specifically for the JS SDK script.

[imbrian]

Good catch. Maybe to line 39 then? At least would give

// resolve with the existing global paypal namespace when a script with the same params already exists
    if (findScript(url, attributes) && existingWindowNamespace) {
        return PromisePonyfill.resolve(existingWindowNamespace);
 }

a chance to gracefully handle an incorrect implementation in production today?

[imbrian]

Oh actually, that wouldn't find the existing instance, because the URL would be different due to missing clientId.

[jshawl]

🚀 one small suggestion about moving the check down but I like the improvement!

[jshawl]

I like how you added what the existing options were, as it will make it easier to identify where the problem can be fixed!

[jshawl]

should the options.clientId check come after line 112? I just noticed that throw new Error("Expected an options object."); will never be hit and/or the error message might be something like "cannot read attribute clientId of undefined"

[spencersablan]

Good thought! We do call validateArguments() on line 16 as well, so we will still get a that Expected an options object error if a developer doesn't pass anything into loadScript()

It is a bit odd that we are calling validateArguments() at the top of both loadScript and loadCustomScript when loadScript uses loadCustomScript... but there's probably a good reason for it. 😅

[jshawl]

ah you're right! pls ignore my comment as 112 will throw before the client id check

[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]

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]

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.