-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
[Stripe-v3] Allow Implicit Elements Resolution #40470
Conversation
Stripe supports implicit elements resolution through createToken. If only options are passed, it will attempt to resolve the element from Stripe.js.
@JakeCooper Thank you for submitting this PR! 🔔 @ejsmith @amritk @AdamCmiel @jleider @galuszkak @slangeder @marlosin @ttmarek @kimehrenpohl @KrishnaPravin @hirochachacha - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings. Let’s review the numbers, shall we? Comparison details 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
@JakeCooper I guess we need to pass at least any 1 element to the createToken method. eg: a CVV element can resolve other elements like card number, expiry, etc |
https://github.com/stripe/react-stripe-elements/blob/master/README.md#server-side-rendering-ssr Did you try it with server side rendering? I'm running that exact code on a new project and it doesn't seem to error. |
After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience! |
If possible can you please send a small working snippet. So that I can run it. |
Sounds like we should hold off on merging until we hear back from @KrishnaPravin? |
If someone else can verify if Stripe supports implicit elements resolution through createToken without at least one stripe element you can definitely merge it. It is just I'm not able to replicate from my side. |
Isn't it about time stripe starts managing these themselves? Can we tag a stripe engineer? |
It will definitely help. Please do it. |
They will be: stripe/stripe-node#736 Not sure what to do with this PR yet, so will hold off on merging. |
@orta that PR is for a different library than this one is; they're orthogonal. |
Hi, I work on Stripe.js I think there's been a bit of a misconception here. This version of I'd recommend taking a look at the DefinitelyTyped defs for React-Stripe-Elements and seeing if that satisfies the need here, looks like it might. |
Sounds like this can be closed then. |
Stripe supports implicit elements resolution through createToken.
If only options are passed, it will attempt to resolve the element from Stripe.js.
Please fill in this template.
npm test
.)npm run lint package-name
(ortsc
if notslint.json
is present).Select one of these and delete the others:
If adding a new definition:
.d.ts
files generated via--declaration
dts-gen --dt
, not by basing it on an existing project.tslint.json
should be present and it shouldn't have any additional or disabling of rules. Just content as{ "extends": "dtslint/dt.json" }
. If for reason the some rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]
and not for whole package so that the need for disabling can be reviewed.tsconfig.json
should havenoImplicitAny
,noImplicitThis
,strictNullChecks
, andstrictFunctionTypes
set totrue
.If changing an existing definition:
tslint.json
containing{ "extends": "dtslint/dt.json" }
. If for reason the any rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]
and not for whole package so that the need for disabling can be reviewed.If removing a declaration:
notNeededPackages.json
.