Skip to content

Refactor the standard integration guide #61

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

Merged
merged 5 commits into from
Aug 15, 2023

Conversation

gregjopa
Copy link
Contributor

The DevRel team has been working on improving the Standard Integration code snippets across developer.paypal.com. This PR includes many of these new changes.

Improvements include:

  • Using the common naming of PAYPAL_CLIENT_ID and PAYPAL_CLIENT_SECRET for env variables.
  • Using a single file for the Node.js server code.
  • Teach negative testing with code comments for using the PayPal-Mock-Response request header for order creation and capture.
  • Always try to relay PayPal API JSON responses and status codes back to the frontend.
  • Death to alert() since it pauses the popup flow. Replacing this with appending UI messages to the button container.

There are some other changes I am less confident on and could use feedback on.

  • What route names should we use for wrapping create order and capture order api calls?
  • Whats the best way to pass the OrderID to the create order api wrapper? Pass it in the URL or in the post body?
  • Should we recommend the Express static module or just return the html file directly from the file system?

Feedback welcome! Let us know what you like and don't like.

Copy link

@wsbrunson wsbrunson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks amazing!

Comment on lines +11 to +12
<script src="https://www.paypal.com/sdk/js?client-id=test&currency=USD"></script>
<script>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing i would love to see is all of our examples using paypal-js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be open to that. How do you think they should use paypal-js in a repo like this? Should we recommend a separate client-side js file that imports in paypal-js and then serve up a bundle via rollup or webpack? Or host it from paypalobjects.com and have folks download it from there?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the later. I think ideally we'd have something that was as easy as stripe's: https://stripe.com/docs/libraries/stripejs-esmodule#web-stripejs-html

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with the former, we have to start talking about build tools and that always sucks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hear ya on the burden of teaching build tools. I like offering both the CDN option and the ES Module like Stripe does. I'll take this paypal-js loader suggestion back to the team to get more feedback.

Copy link
Contributor

@jshawl jshawl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Left a few non-blocking comments :shipit:

// parse post params sent in body in json format
app.use(express.json());

app.post("/my-server/create-paypal-order", async (req, res) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 this tripped me up the other day thinking my-server was not an actual route...

@gregjopa
Copy link
Contributor Author

Thanks for the feedback everyone! I applied some of the recommendations and I'm going to merge this PR.

@cnallam, fyi I changed two things with the codespaces integration for standard checkout:

  1. There was a bug with the workspace path that is now updated.
  2. The env variables have been renamed to PAYPAL_CLIENT_ID and PAYPAL_CLIENT_SECRET to better match the PayPal Auth docs and to help the developer know that these credentials are for PayPal. We plan to make the same change to the Advanced Integration in a follow up PR.

I've successfully tested the CodeSpaces integration off this feature branch. Here are some screenshots for reference:

Screenshot 2023-08-15 at 9 48 08 AM Screenshot 2023-08-15 at 9 51 14 AM Screenshot 2023-08-15 at 9 51 21 AM

@gregjopa gregjopa merged commit e859c6f into main Aug 15, 2023
@gregjopa gregjopa deleted the refactor-standard-integration-guide branch August 15, 2023 14:56
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.

5 participants