Remove "curried" nested resources and manually specified urlParams #625

rattrayalex-stripe · 2019-05-10T05:10:41Z

r? @ob-stripe
cc @irace-stripe @paulasjes-stripe @robz-stripe @remi-stripe
cc @stripe/api-libraries

This removes support for "curried" url parameters on nested resources, like this:

const transferReversals = new resources.TransferReversal(stripe, {transferId: 'tr_123'});
transferReversals.update('trr_123', params);

and along with it, removing the associated resource objects (ie; stripe.transferReversals no longer exists) on the grounds that they were not usable outside of the curried case.

You can still do this instead:

stripe.transters.updateReversal('tr_123', 'tr_123', params);

This applies to all nested resources, namely:

Persons
LoginLinks
SubscriptionScheduleRevisions
ApplicationFeeRefunds
TaxIds
TransferReversals

This means the names of url parameters is no longer meaningful, and can be changed without compatibility concerns. We can even have two url parameters named id if we like (though I think that would be less clear).

Because of this, and because we no longer have any paths specified as a function, I was also able to remove urlParams from the stripeMethods, inferring them with a regex instead (though their name values are not actually used; only the number of url params is meaningful).

Note that we briefly experimented with support for this pattern:

stripe.transferReversals.update('tr_123', 'trr_123', params);

and ultimately decided against it for now; we may reconsider in the future. Thoughts from the community welcome!

Verified this is no different with: ```js const urlParams = utils.extractUrlParams(spec.path || ''); if ( !(spec.urlParams || []).every((x, i) => urlParams[i] === x) || (spec.urlParams || []).length !== urlParams.length ) { throw Error( 'mismatch' + JSON.stringify(urlParams) + JSON.stringify(spec.urlParams) ); } ``` inside StripeMethod

rattrayalex-stripe · 2019-05-10T05:13:33Z

lib/makeRequest.js


 function getRequestOpts(self, requestArgs, spec, overrideData) {
  // Extract spec values with defaults.
-  const commandPath =
-    typeof spec.path == 'function'


we had no paths specified as functions anymore

rattrayalex-stripe · 2019-05-10T05:13:43Z

lib/makeRequest.js

@@ -1,14 +1,10 @@
 'use strict';

 const utils = require('./utils');
-const OPTIONAL_REGEX = /^optional!/;


we had no url params specified as optional anymore

rattrayalex-stripe · 2019-05-10T05:25:01Z

lib/makeRequest.js

-      throw new Error(
-        `Stripe: Argument "${
-          urlParams[i]
-        }" required, but got: ${arg} (on API request to \`${requestMethod} ${path}\`)`


this error wasn't as good as the other, combined it.

rattrayalex-stripe · 2019-05-10T05:25:24Z

lib/makeRequest.js

-
-    if (param == 'id' && typeof arg !== 'string') {
-      path = self.createResourcePathWithSymbols(spec.path);
+  const urlData = urlParams.reduce((urlData, param) => {


Now that we're on Node 6+, we can use "modern" features like .reduce

robz-stripe · 2019-05-10T13:12:35Z

lib/resources/Disputes.js

@@ -8,5 +8,5 @@ module.exports = StripeResource.extend({

  includeBasic: ['list', 'retrieve', 'update'],

-  close: stripeMethod({method: 'POST', path: '/{id}/close', urlParams: ['id']}),
+  close: stripeMethod({method: 'POST', path: '/{id}/close'}),


Should we break this out onto multiple lines like we do with most of the stripeMethod() calls? Purely a style thing.

Good call! prettier collapsed them but we can uncollapse.

cc @irace-stripe we may want to try to indicate to prettier-poet that these should break.

I think <Node.Object entries={} break /> would be a useful API, since prettier generally allows objects to be configured to be broken

robz-stripe · 2019-05-10T13:12:56Z

lib/resources/Orders.js

@@ -8,11 +8,10 @@ module.exports = StripeResource.extend({

  includeBasic: ['create', 'list', 'retrieve', 'update'],

-  pay: stripeMethod({method: 'POST', path: '/{id}/pay', urlParams: ['id']}),
+  pay: stripeMethod({method: 'POST', path: '/{id}/pay'}),


Same question here on one vs. multiple lines.

This reverts commit d88a3e7.

…monstrate their use

ob-stripe · 2019-05-10T17:19:44Z

Let's wait until we reach a decision re. nested resources, but this looks pretty great to me!

This reverts commit e5eccb8.

rattrayalex-stripe · 2019-05-10T19:35:24Z

Per internal discussion document, I've reverted my change adding support for stripe.transferReversals.update('tr_123', 'trr_123', params); which I feel good about.

rattrayalex-stripe · 2019-05-10T23:03:21Z

Per IRL with @ob-stripe merging this.

https://github.com/stripe/stripe-node/pull/625/files#diff-d3dd6c4fd6f915f29d42e4081dc817a8L85

* Extract separate resources file (to aid with code generation) * Remove resources that were removed in #625 https://github.com/stripe/stripe-node/pull/625/files#diff-d3dd6c4fd6f915f29d42e4081dc817a8L85

) * Drop support for optional url params * Delete nested resource files * Remove urlData * Extract urlParams from path instead of manual definition Verified this is no different with: ```js const urlParams = utils.extractUrlParams(spec.path || ''); if ( !(spec.urlParams || []).every((x, i) => urlParams[i] === x) || (spec.urlParams || []).length !== urlParams.length ) { throw Error( 'mismatch' + JSON.stringify(urlParams) + JSON.stringify(spec.urlParams) ); } ``` inside StripeMethod * Remove manually specified urlParams * Add a deprecation error message * Revert "Delete nested resource files" This reverts commit d88a3e7. * Fix nested resources for non-curried urlParams and update tests to demonstrate their use * Refactor makeRequest * Revert "Revert "Delete nested resource files"" This reverts commit e5eccb8.

* Extract separate resources file (to aid with code generation) * Remove resources that were removed in #625 https://github.com/stripe/stripe-node/pull/625/files#diff-d3dd6c4fd6f915f29d42e4081dc817a8L85

) * Drop support for optional url params * Delete nested resource files * Remove urlData * Extract urlParams from path instead of manual definition Verified this is no different with: ```js const urlParams = utils.extractUrlParams(spec.path || ''); if ( !(spec.urlParams || []).every((x, i) => urlParams[i] === x) || (spec.urlParams || []).length !== urlParams.length ) { throw Error( 'mismatch' + JSON.stringify(urlParams) + JSON.stringify(spec.urlParams) ); } ``` inside StripeMethod * Remove manually specified urlParams * Add a deprecation error message * Revert "Delete nested resource files" This reverts commit d88a3e7. * Fix nested resources for non-curried urlParams and update tests to demonstrate their use * Refactor makeRequest * Revert "Revert "Delete nested resource files"" This reverts commit e5eccb8.

* Extract separate resources file (to aid with code generation) * Remove resources that were removed in #625 https://github.com/stripe/stripe-node/pull/625/files#diff-d3dd6c4fd6f915f29d42e4081dc817a8L85

) * Drop support for optional url params * Delete nested resource files * Remove urlData * Extract urlParams from path instead of manual definition Verified this is no different with: ```js const urlParams = utils.extractUrlParams(spec.path || ''); if ( !(spec.urlParams || []).every((x, i) => urlParams[i] === x) || (spec.urlParams || []).length !== urlParams.length ) { throw Error( 'mismatch' + JSON.stringify(urlParams) + JSON.stringify(spec.urlParams) ); } ``` inside StripeMethod * Remove manually specified urlParams * Add a deprecation error message * Revert "Delete nested resource files" This reverts commit d88a3e7. * Fix nested resources for non-curried urlParams and update tests to demonstrate their use * Refactor makeRequest * Revert "Revert "Delete nested resource files"" This reverts commit e5eccb8.

* Extract separate resources file (to aid with code generation) * Remove resources that were removed in #625 https://github.com/stripe/stripe-node/pull/625/files#diff-d3dd6c4fd6f915f29d42e4081dc817a8L85

) * Drop support for optional url params * Delete nested resource files * Remove urlData * Extract urlParams from path instead of manual definition Verified this is no different with: ```js const urlParams = utils.extractUrlParams(spec.path || ''); if ( !(spec.urlParams || []).every((x, i) => urlParams[i] === x) || (spec.urlParams || []).length !== urlParams.length ) { throw Error( 'mismatch' + JSON.stringify(urlParams) + JSON.stringify(spec.urlParams) ); } ``` inside StripeMethod * Remove manually specified urlParams * Add a deprecation error message * Revert "Delete nested resource files" This reverts commit d88a3e7. * Fix nested resources for non-curried urlParams and update tests to demonstrate their use * Refactor makeRequest * Revert "Revert "Delete nested resource files"" This reverts commit e5eccb8.

* Extract separate resources file (to aid with code generation) * Remove resources that were removed in #625 https://github.com/stripe/stripe-node/pull/625/files#diff-d3dd6c4fd6f915f29d42e4081dc817a8L85

* Update version to 7 and drop support for Node 4 and Node 5, and Node 7 * Format using Prettier tmp * Alphabetize basic methods * Use 'id' for all single identifier positional arguments * Fix typo * Modernize ES5 to ES6 with lebab (#607) * Add lebab and a script to run it * lebab transform: arrow * lebab transform: arg-rest * lebab transform: arg-spread * lebab transform: obj-method * lebab transform: obj-shorthand * lebab transform: let * lebab transform: template * lebab transform: default-param * lebab transform: destruct-param * lebab transform: includes * Revert "Add lebab and a script to run it" This reverts commit 70fd492. * Revert "lebab transform: destruct-param" because its changes didn't seem good. This reverts commit b56f52d. * Revert "lebab transform: default-param" because it seems dangerous / backwards-incompatible. This reverts commit 7eba992. * Unrelated: mark 8.1 as minimum 8-series version * Add mocha-only script * Use arrows in more places * Loosen some eslint rules I don't love * Remove deprecated methods * Add VSCode and EditorConfig files * Bump dependencies to latest versions * Remove legacy parameter support in invoices.retrieveUpcoming() * Misc. manual formatting (#623) * Misc. manual formatting * Fix some unit tests * Roll back path argument name changes * Misc. manual formatting * Remove "curried" nested resources and manually specified urlParams (#625) * Drop support for optional url params * Delete nested resource files * Remove urlData * Extract urlParams from path instead of manual definition Verified this is no different with: ```js const urlParams = utils.extractUrlParams(spec.path || ''); if ( !(spec.urlParams || []).every((x, i) => urlParams[i] === x) || (spec.urlParams || []).length !== urlParams.length ) { throw Error( 'mismatch' + JSON.stringify(urlParams) + JSON.stringify(spec.urlParams) ); } ``` inside StripeMethod * Remove manually specified urlParams * Add a deprecation error message * Revert "Delete nested resource files" This reverts commit d88a3e7. * Fix nested resources for non-curried urlParams and update tests to demonstrate their use * Refactor makeRequest * Revert "Revert "Delete nested resource files"" This reverts commit e5eccb8. * Extract resources file (to aid with code generation) (#626) * Extract separate resources file (to aid with code generation) * Remove resources that were removed in #625 https://github.com/stripe/stripe-node/pull/625/files#diff-d3dd6c4fd6f915f29d42e4081dc817a8L85 * Update CHANGELOG for 7.0.0 release

rattrayalex-stripe added 4 commits May 9, 2019 18:48

Drop support for optional url params

b8cbd25

Delete nested resource files

d88a3e7

Remove urlData

b3cfe88

rattrayalex-stripe mentioned this pull request May 10, 2019

[WIP] Rename more parameters to match OpenAPI #624

Closed

stripe-ci assigned ob-stripe May 10, 2019

rattrayalex-stripe commented May 10, 2019

View reviewed changes

robz-stripe reviewed May 10, 2019

View reviewed changes

rattrayalex-stripe added 5 commits May 10, 2019 08:50

Remove manually specified urlParams

7842939

Add a deprecation error message

e52219b

Revert "Delete nested resource files"

e5eccb8

This reverts commit d88a3e7.

Fix nested resources for non-curried urlParams and update tests to de…

71d5200

…monstrate their use

Refactor makeRequest

f650b77

rattrayalex-stripe force-pushed the ralex/remove-url-data branch from a1465f6 to f650b77 Compare May 10, 2019 15:50

rattrayalex-stripe mentioned this pull request May 10, 2019

stripe-node v7.0.0 #606

Merged

13 tasks

ob-stripe added the breaking-api-change label May 10, 2019

Revert "Revert "Delete nested resource files""

d58bbf8

This reverts commit e5eccb8.

rattrayalex-stripe changed the title ~~Remove curried urlData and manually specified urlParams~~ Remove "curried" nested resources and manually specified urlParams May 10, 2019

rattrayalex-stripe force-pushed the ralex/remove-url-data branch from 1cdba64 to d58bbf8 Compare May 10, 2019 19:34

rattrayalex-stripe added the approved label May 10, 2019

rattrayalex-stripe merged commit cf062aa into v7.0.0 May 10, 2019

rattrayalex-stripe deleted the ralex/remove-url-data branch May 10, 2019 23:03

rattrayalex-stripe added a commit that referenced this pull request May 10, 2019

Remove resources that were removed in #625

812fb7b

https://github.com/stripe/stripe-node/pull/625/files#diff-d3dd6c4fd6f915f29d42e4081dc817a8L85

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove "curried" nested resources and manually specified urlParams #625

Remove "curried" nested resources and manually specified urlParams #625

rattrayalex-stripe commented May 10, 2019 •

edited

rattrayalex-stripe May 10, 2019

rattrayalex-stripe May 10, 2019

rattrayalex-stripe May 10, 2019

rattrayalex-stripe May 10, 2019

robz-stripe May 10, 2019

rattrayalex-stripe May 10, 2019

rattrayalex-stripe May 10, 2019

rattrayalex-stripe May 10, 2019

robz-stripe May 10, 2019

ob-stripe commented May 10, 2019

rattrayalex-stripe commented May 10, 2019

rattrayalex-stripe commented May 10, 2019

Remove "curried" nested resources and manually specified urlParams #625

Remove "curried" nested resources and manually specified urlParams #625

Conversation

rattrayalex-stripe commented May 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ob-stripe commented May 10, 2019

rattrayalex-stripe commented May 10, 2019

rattrayalex-stripe commented May 10, 2019

rattrayalex-stripe commented May 10, 2019 •

edited