New advanced integration with new card fields #53
Conversation
jshawl
left a comment
jshawl
left a comment
There was a problem hiding this comment.
Looks great! I did not yet run this locally but can if you want - just lmk. I also left a few non-blocking comments for where I think some error handling can be removed. I think it would be helpful to include a readme in advanced-integration and also a deprecation notice in advance-integration-legacy <- just food for thought! Otherwise, 
!
|
|
||
| ## Instructions | ||
|
|
||
| 1. Rename `.env.example` to `.env` and update `CLIENT_ID` and `APP_SECRET`. |
There was a problem hiding this comment.
I don't think there is an .env.example in this directory (advanced-integration-legacy)
| throw new Error(errorMessage); | ||
| } | ||
| const error = new Error (await response.text()) | ||
| error.status = response.status |
| res.render("checkout", { clientId, clientToken }); | ||
| res.render("index", { clientId }); | ||
| } catch (err) { | ||
| handleError(res, err); |
There was a problem hiding this comment.
Will res.render ever throw?
There was a problem hiding this comment.
its possible. I think that try/catch was there for paypa.generateClientToken but its possible there could be an issue rendering index.ejs or finding the file if the filesystem changes
| }); | ||
|
|
||
| app.get("/purchase", async (req, res) => { | ||
| const clientId = process.env.CLIENT_ID; |
There was a problem hiding this comment.
option to add this to const { PORT = 8888 } = process.env; above
| @@ -1 +0,0 @@ | |||
| package-lock=false | |||
There was a problem hiding this comment.
We may want to keep the .npmrc file and have it contain the following setting:
registry=https://registry.npmjs.org
That way we can always ensure any package-lock urls point to the public npm registry and not our private paypal registry.
I'm fine with adding lock files. Just want to make sure they always point to public npm.
|
This PR looks good! I do think we should keep it as a draft PR until the public docs are upgraded with the new card fields. Right now, the current advanced integration doc has a link to this "advanced integration" folder inside this repo and that will break if we merge this PR: https://developer.paypal.com/docs/checkout/advanced/integrate#link-generateclienttoken |
agreed! I've looped in Lance Han from the devrel team, they are going to give this a look and spin it up. I'll work with them to possibly take over the merging of this PR to coincide with their docs changes |
|
I have a draft PR for adding the new card-fields component here in #84 |
Two big changes
advanced-integrationmoved toadvanced-integration-legacy(Old hosted card fields)advanced-integrationIn addition
paypal-api.js